Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Let user take screenshots when reporting Android webcompat issues #25584

Conversation

vadimstruts
Copy link
Collaborator

@vadimstruts vadimstruts commented Sep 16, 2024

Resolves brave/brave-browser#36486

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally:
    • npm run test -- brave_browser_tests, npm run test -- brave_unit_tests wiki
    • npm run presubmit wiki, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

Case 1

  1. Go to any web page
  2. Open Brave Shields pop up dialog.
  3. Switch off Brave Shields if It is ON.
  4. Click "Report broken site".
  5. Click "Submit"
  6. Make sure screenshot has not been sent with the report.

Case 2

  1. Go to any web page
  2. Open Brave Shields pop up dialog.
  3. Switch off Brave Shields if It is ON.
  4. Click "Report broken site".
  5. Click the checkbox "Include screenshot of the current page"
  6. Click "View screenshot" to check the screenshot of the current page is ready
  7. Click "Submit"
  8. Make sure the screenshot has been sent in context of the webcompat report

@vadimstruts vadimstruts force-pushed the 36486-let-user-take-screenshots-when-reporting-android-webcompat-issues branch from 8b220be to ea9fce7 Compare September 17, 2024 11:27
@vadimstruts vadimstruts marked this pull request as ready for review September 17, 2024 14:22
@vadimstruts vadimstruts requested review from deeppandya and a team as code owners September 17, 2024 14:22
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strings++

protected void onPostExecute(byte[] pngBytes) {
assert ThreadUtils.runningOnUiThread();

if (isCancelled()) return;
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash Sep 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be here something like

if (isCancelled()) {
   mCallback.OnScrrenshotReady(pngBytes);
   return;
}

?

Copy link
Contributor

@AlexeyBarabash AlexeyBarabash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍
lgtm with few minor notices

@vadimstruts vadimstruts force-pushed the 36486-let-user-take-screenshots-when-reporting-android-webcompat-issues branch from 40b6dd8 to e466787 Compare September 18, 2024 11:55
private static final String TAG = "ShieldsScreenshot";
private final BraveScreenshotRunnable mBraveScreenshotRunnable;

private static class PngConvertorTask extends AsyncTask<byte[]> {
Copy link
Contributor

@deeppandya deeppandya Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Asynctask is deprecated with api 30 : https://developer.android.com/reference/android/os/AsyncTask.html. we should use executors here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

@deeppandya deeppandya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

return;
}

ExecutorService es = Executors.newSingleThreadExecutor();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @vadimstruts for the changes here.

@vadimstruts vadimstruts force-pushed the 36486-let-user-take-screenshots-when-reporting-android-webcompat-issues branch from 7b10495 to 297a0a1 Compare September 19, 2024 20:54
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
Signed-off-by: Vadym Struts <vstruts@brave.com>
@vadimstruts vadimstruts force-pushed the 36486-let-user-take-screenshots-when-reporting-android-webcompat-issues branch from 297a0a1 to 648d214 Compare September 20, 2024 10:02
@vadimstruts vadimstruts merged commit 7aa11e8 into master Sep 20, 2024
17 checks passed
@vadimstruts vadimstruts deleted the 36486-let-user-take-screenshots-when-reporting-android-webcompat-issues branch September 20, 2024 11:03
@github-actions github-actions bot added this to the 1.72.x - Nightly milestone Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let user take screenshots when reporting Android webcompat issues
5 participants