Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2534333002:
Show 'Send Feedback' button on the sad tab page on multiple failures (Closed)
DescriptionShow 'Send Feedback' on the sad tab page on multiple failures
Fixed bug 665237
Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well.
BUG=665237
Committed: https://crrev.com/e3f3136a52ce8425f1a67d470203f83f8e961245
Cr-Commit-Position: refs/heads/master@{#435774}
Patch Set 1 #
Total comments: 40
Patch Set 2 : Updated based on twellington's comments #
Total comments: 14
Patch Set 3 : Modified based off of sdefresne's comments #Patch Set 4 : Fixed based of twellington's most recent comments. #
Total comments: 16
Patch Set 5 : Fixed code based off of twellington's comments. #
Messages
Total messages: 46 (26 generated)
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
jwanda@chromium.org changed reviewers: + twellington@chromium.org
Hey Teresa! I'm messaging you since you seem to have had the most recent and significant change in my code. When you have the time would you be able to look at this cl? I'll add an owner for all my code in a little bit. Thanks! JJ https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:117: simulateRendererKilled(tab, true); Please correct me if I'm wrong here.
On 2016/11/30 18:12:16, JJ wrote: > Hey Teresa! > > I'm messaging you since you seem to have had the most recent and significant > change in my code. When you have the time would you be able to look at this cl? > I'll add an owner for all my code in a little bit. > > Thanks! > JJ > > https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... > File > chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java > (right): > > https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... > chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:117: > simulateRendererKilled(tab, true); > Please correct me if I'm wrong here. Drive by comment. The title of your change could use a bit of TLC (you can edit it via the "Edit Issue" link in codereview). In general, the first line should clearly describe the change being made. Often when we are tracking down a particular change we are scanning just this line for 100s of changes. Fix bug XXXX doesn't give you much insight there. A clearer message could be: "Show 'Send Feedback' on the sad tab page on multiple failures" or something like that. Here's a link about Chromium recommendations about cl descriptions: https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list...
jwanda@chromium.org changed reviewers: + dfalcantara@chromium.org, sdefresne@chromium.org
sdefresne@chromium.org: Please review changes in components/new_or_sad_tab_string.grdp I added a string that is displayed in Android when sad tab dies twice. I added the string in here since it was directly related to sad_tab and the sad tab strings in android are pulled from here. Click on the bug link for a little more information. dfalcantara@chromium.org: Please review changes in chrome/android/ When you have the chance I wanted you to take a look on the code and confirm everything is in order. Thanks! JJ W
Description was changed from ========== Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 ========== to ========== Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 ==========
On 2016/11/30 18:30:58, Ted C wrote: > On 2016/11/30 18:12:16, JJ wrote: > > Hey Teresa! > > > > I'm messaging you since you seem to have had the most recent and significant > > change in my code. When you have the time would you be able to look at this > cl? > > I'll add an owner for all my code in a little bit. > > > > Thanks! > > JJ > > > > > https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... > > File > > chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java > > (right): > > > > > https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... > > > chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:117: > > simulateRendererKilled(tab, true); > > Please correct me if I'm wrong here. > > Drive by comment. > > The title of your change could use a bit of TLC (you can edit it via the > "Edit Issue" link in codereview). In general, the first line should > clearly describe the change being made. Often when we are tracking down > a particular change we are scanning just this line for 100s of changes. > > Fix bug XXXX doesn't give you much insight there. > > A clearer message could be: > "Show 'Send Feedback' on the sad tab page on multiple failures" or something > like that. > > Here's a link about Chromium recommendations about cl descriptions: > https://www.chromium.org/developers/contributing-code#TOC-Writing-change-list... Thanks! Didn't know what the best way was to do this. Changed the title.
All of the nits on code comments are my personal preference. It's up to you if/how you want to change them. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:31: public static View createSadTabView( In the current code base #createSadTabView() is only called once, so I don't think this helper method is needed. Where is it used? https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:44: * @param buttonText The label for the "reloadButton" nit: add a period at the end of this description. I would word this as "The label for the Reload button." to better match the description for reloadButtonAction. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:49: OnClickListener reloadButtonAction, String buttonText) { Since this already takes the context, it could just take the resource id for the string and retrieve it from the context. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:373: private int mSadTabSuccessiveRefreshCounter; nit: "Counts the number of successive refreshes on the sad tab page. This is reset after a successful page load." or something like that. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1670: // a page. nit: "Reset the sad tab refresh counter since the page successfully loaded." https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1890: // and change the onClickListener on the off The end of this comment doesn't make sense. I think it should just have a period after onClickListener and drop the rest. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1898: // actually being a "menu" or "keyboard" action but still a user action. We probably want to introduce a new user action when the user taps "Send Feedback" on the sad tab page so that we can track how often this feature is used separately from how often the option is chosen from the menu. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1909: mThemedApplicationContext, suggestionAction, reloadButtonAction, buttonText); I think this logic could be streamlined a bit by using a final boolean to decide whether the send feedback button is showing. final boolean showSendFeedbackButton = mSadTabSuccessiveRefreshCounter >= 1; OnClickListener reloadButtonAction = new OnClickListener() { @Override public void onClick(View view) { if (showSendFeedbackButton) { // logic to send feedback } else { reload(); } } }; .... mSadTabView = SadTabViewFactory.createSadTab(...., showSendFeedbackButton ? R.string.sad_tab_send_feedback_label : R.string.sad_tab_reload_button); https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1935: * This first removes any existing sad tab view and shows it again. We use this to "reload" nit: "Removes any existing sad tab view then shows it again. This "reloads" the tab without going through any formal loading logic." I don't think the last sentence is necessary. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:54: * background or the renderer was killed by the OS out-of-memory killer. nit: change this back (this was probably a side-effect of 'git cl format') https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:66: tab.simulateRendererKilledForTesting(false); Change this to use the new helper method (same above) https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:75: * button from "Reload" to "Send Feedback". nit: move some of the text from the bottom line up since the line length in Java is 100 characters. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:86: assertEquals("Expected the feedback tab to have the reload label", actualText, "Expected the sad tab button to have the reload label"? Also, flip the actual text and expected text so that it's assertEquals(expected, actual) https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:108: nit: remove extra blank line https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:117: simulateRendererKilled(tab, true); On 2016/11/30 18:12:16, JJ wrote: > Please correct me if I'm wrong here. simulateRendererKilled() below is running on the UI thread blocking. I don't think you can can call runOnUiThreadBlocking() inside of another runOnUiThreadBlocking() runnable. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:136: * Shortcut method that reloads a tab with a SadTabView currently displayed nit: "Helper method that reloads a tab that is already displaying the SadTabView." https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:140: nit: remove blank line (here and in simulateRendererKilled above) https://codereview.chromium.org/2534333002/diff/1/components/new_or_sad_tab_s... File components/new_or_sad_tab_strings.grdp (right): https://codereview.chromium.org/2534333002/diff/1/components/new_or_sad_tab_s... components/new_or_sad_tab_strings.grdp:34: <message name="IDS_SAD_TAB_SEND_FEEDBACK_LABEL" desc="Button label in the sad tab page for sending feedback. This label replaces the reload button after a crash happens twice" formatter_data="android_java"> nit: "... happens twice in a row."
Description was changed from ========== Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 ========== to ========== Show 'Send Feedback' on the sad tab page on multiple failures Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 ==========
Alright updated cl based off of comments. Asked a few questions in them so please reply when you have the chance. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:31: public static View createSadTabView( On 2016/11/30 19:42:38, Theresa Wellington wrote: > In the current code base #createSadTabView() is only called once, so I don't > think this helper method is needed. Where is it used? Nowhere else since I changed the method.. I'll go ahead and delete it. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:44: * @param buttonText The label for the "reloadButton" On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: add a period at the end of this description. I would word this as "The > label for the Reload button." to better match the description for > reloadButtonAction. Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:49: OnClickListener reloadButtonAction, String buttonText) { On 2016/11/30 19:42:38, Theresa Wellington wrote: > Since this already takes the context, it could just take the resource id for the > string and retrieve it from the context. Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:373: private int mSadTabSuccessiveRefreshCounter; On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: "Counts the number of successive refreshes on the sad tab page. This is > reset after a successful page load." or something like that. Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1670: // a page. On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: "Reset the sad tab refresh counter since the page successfully loaded." Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1890: // and change the onClickListener on the off On 2016/11/30 19:42:38, Theresa Wellington wrote: > The end of this comment doesn't make sense. I think it should just have a period > after onClickListener and drop the rest. Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1898: // actually being a "menu" or "keyboard" action but still a user action. On 2016/11/30 19:42:38, Theresa Wellington wrote: > We probably want to introduce a new user action when the user taps "Send > Feedback" on the sad tab page so that we can track how often this feature is > used separately from how often the option is chosen from the menu. Tell me if this is the way you interpreted it. https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1909: mThemedApplicationContext, suggestionAction, reloadButtonAction, buttonText); On 2016/11/30 19:42:38, Theresa Wellington wrote: > I think this logic could be streamlined a bit by using a final boolean to decide > whether the send feedback button is showing. > > final boolean showSendFeedbackButton = mSadTabSuccessiveRefreshCounter >= 1; > OnClickListener reloadButtonAction = new OnClickListener() { > @Override > public void onClick(View view) { > if (showSendFeedbackButton) { > // logic to send feedback > } else { > reload(); > } > } > }; > > .... > > mSadTabView = SadTabViewFactory.createSadTab(...., > showSendFeedbackButton ? R.string.sad_tab_send_feedback_label > : R.string.sad_tab_reload_button); Alright changed it to be almost exactly that, tell me if there is something that you feel is off! https://codereview.chromium.org/2534333002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1935: * This first removes any existing sad tab view and shows it again. We use this to "reload" On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: "Removes any existing sad tab view then shows it again. This "reloads" the > tab without going through any formal loading logic." > > I don't think the last sentence is necessary. Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:54: * background or the renderer was killed by the OS out-of-memory killer. On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: change this back (this was probably a side-effect of 'git cl format') Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:75: * button from "Reload" to "Send Feedback". On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: move some of the text from the bottom line up since the line length in Java > is 100 characters. Yeah, weird. I have it set here and I thought the formatted would notice. Don't know why it moved it down like this. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:86: assertEquals("Expected the feedback tab to have the reload label", actualText, On 2016/11/30 19:42:38, Theresa Wellington wrote: > "Expected the sad tab button to have the reload label"? > > Also, flip the actual text and expected text so that it's assertEquals(expected, > actual) Yup, done. Thanks for some reason I flip them in my head pretty often. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:108: On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: remove extra blank line Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:117: simulateRendererKilled(tab, true); On 2016/11/30 19:42:38, Theresa Wellington wrote: > On 2016/11/30 18:12:16, JJ wrote: > > Please correct me if I'm wrong here. > > simulateRendererKilled() below is running on the UI thread blocking. I don't > think you can can call runOnUiThreadBlocking() inside of another > runOnUiThreadBlocking() runnable. Oh sorry not like that. I did try to just add tab.simulateRendererKilledForTesting right after tab.didFinishPageLoad() but that would cause the test to fail. Still not entirely sure why. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:136: * Shortcut method that reloads a tab with a SadTabView currently displayed On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: "Helper method that reloads a tab that is already displaying the > SadTabView." Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:140: On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: remove blank line (here and in simulateRendererKilled above) Done. https://codereview.chromium.org/2534333002/diff/1/components/new_or_sad_tab_s... File components/new_or_sad_tab_strings.grdp (right): https://codereview.chromium.org/2534333002/diff/1/components/new_or_sad_tab_s... components/new_or_sad_tab_strings.grdp:34: <message name="IDS_SAD_TAB_SEND_FEEDBACK_LABEL" desc="Button label in the sad tab page for sending feedback. This label replaces the reload button after a crash happens twice" formatter_data="android_java"> On 2016/11/30 19:42:38, Theresa Wellington wrote: > nit: "... happens twice in a row." Done.
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2534333002/diff/20001/components/new_or_sad_t... File components/new_or_sad_tab_strings.grdp (right): https://codereview.chromium.org/2534333002/diff/20001/components/new_or_sad_t... components/new_or_sad_tab_strings.grdp:34: <message name="IDS_SAD_TAB_SEND_FEEDBACK_LABEL" desc="Button label in the sad tab page for sending feedback. This label replaces the reload button after a crash happens twice in a row." formatter_data="android_java"> As this string is only used on android, please put it in an "<if expr>" block: <if expr="is_android"> <message ... </if>
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:75: * button from "Reload" to "Send Feedback". On 2016/11/30 23:55:53, JJ wrote: > On 2016/11/30 19:42:38, Theresa Wellington wrote: > > nit: move some of the text from the bottom line up since the line length in > Java > > is 100 characters. > > Yeah, weird. I have it set here and I thought the formatted would notice. Don't > know why it moved it down like this. git cl format doesn't work so well on Java files, particularly for comment wrapping. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:117: simulateRendererKilled(tab, true); On 2016/11/30 23:55:54, JJ wrote: > On 2016/11/30 19:42:38, Theresa Wellington wrote: > > On 2016/11/30 18:12:16, JJ wrote: > > > Please correct me if I'm wrong here. > > > > simulateRendererKilled() below is running on the UI thread blocking. I don't > > think you can can call runOnUiThreadBlocking() inside of another > > runOnUiThreadBlocking() runnable. > > Oh sorry not like that. I did try to just add > tab.simulateRendererKilledForTesting right after tab.didFinishPageLoad() but > that would cause the test to fail. Still not entirely sure why. I think it's fine to leave the test like this (I would remove the comment), or I'm happy to take a look at the crash stack and help figure out what's going wrong. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1732: * @param currentTab The tab that the user is currently on. Assumes it cannot be null Typically we annotate parameters that may be null with @Nullable rather than commenting on parameters that cannot be null. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1733: * @param recordAction The label we use to record the user action. nit: "The user action to record"? https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1735: public void startHelpAndFeedback(Tab currentTab, String recordAction) { I like the introduction of this helper method. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:47: reloadButton.setText(context.getString(buttonTextId)); Looking at this again, it can just be "reloadButton.setText(buttonTextId)" since Button has a setText that takes a resource id. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1887: getActivity().startHelpAndFeedback(Tab.this, "MobileSadTabFeedback"); Thanks for adding the new user action. MobileSadTabFeedback also needs an entry in tools/metrics/actions/actions.xml https://codereview.chromium.org/2534333002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:42: ThreadUtils.runOnUiThreadBlocking(new Runnable() { nit: Can this use the new helper method simulateRendererKilled() too? (same for testSadTabNotShownWhen... as well)
nits on the CLs description "Fixed bug 665237" -- This line isn't necessary since BUG= below says the same thing "... emulated this feature here as well." -- It's not clear what "here" means without looking at the files modified. It'd be more clear to say "... emulated this feature on Android as well."
https://codereview.chromium.org/2534333002/diff/20001/components/new_or_sad_t... File components/new_or_sad_tab_strings.grdp (right): https://codereview.chromium.org/2534333002/diff/20001/components/new_or_sad_t... components/new_or_sad_tab_strings.grdp:34: <message name="IDS_SAD_TAB_SEND_FEEDBACK_LABEL" desc="Button label in the sad tab page for sending feedback. This label replaces the reload button after a crash happens twice in a row." formatter_data="android_java"> On 2016/12/01 10:29:34, sdefresne wrote: > As this string is only used on android, please put it in an "<if expr>" block: > > <if expr="is_android"> > <message ... > </if> Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Alright, fixed all the comments you've asked for! https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:66: tab.simulateRendererKilledForTesting(false); On 2016/11/30 19:42:38, Theresa Wellington wrote: > Change this to use the new helper method (same above) Done. https://codereview.chromium.org/2534333002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:117: simulateRendererKilled(tab, true); On 2016/12/01 16:21:21, Theresa Wellington wrote: > On 2016/11/30 23:55:54, JJ wrote: > > On 2016/11/30 19:42:38, Theresa Wellington wrote: > > > On 2016/11/30 18:12:16, JJ wrote: > > > > Please correct me if I'm wrong here. > > > > > > simulateRendererKilled() below is running on the UI thread blocking. I don't > > > think you can can call runOnUiThreadBlocking() inside of another > > > runOnUiThreadBlocking() runnable. > > > > Oh sorry not like that. I did try to just add > > tab.simulateRendererKilledForTesting right after tab.didFinishPageLoad() but > > that would cause the test to fail. Still not entirely sure why. > > I think it's fine to leave the test like this (I would remove the comment), or > I'm happy to take a look at the crash stack and help figure out what's going > wrong. I removed the comment. It's an interesting error for sure though. Don't worry about looking through it though. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1732: * @param currentTab The tab that the user is currently on. Assumes it cannot be null On 2016/12/01 16:21:21, Theresa Wellington wrote: > Typically we annotate parameters that may be null with @Nullable rather than > commenting on parameters that cannot be null. Ah, so in this case I can remove it and don't have to put "NonNull" as an annotation? https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1733: * @param recordAction The label we use to record the user action. On 2016/12/01 16:21:21, Theresa Wellington wrote: > nit: "The user action to record"? Done. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1735: public void startHelpAndFeedback(Tab currentTab, String recordAction) { On 2016/12/01 16:21:21, Theresa Wellington wrote: > I like the introduction of this helper method. c: https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/SadTabViewFactory.java:47: reloadButton.setText(context.getString(buttonTextId)); On 2016/12/01 16:21:21, Theresa Wellington wrote: > Looking at this again, it can just be "reloadButton.setText(buttonTextId)" since > Button has a setText that takes a resource id. Oh you're absolutely right. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1887: getActivity().startHelpAndFeedback(Tab.this, "MobileSadTabFeedback"); On 2016/12/01 16:21:21, Theresa Wellington wrote: > Thanks for adding the new user action. MobileSadTabFeedback also needs an entry > in tools/metrics/actions/actions.xml Done. https://codereview.chromium.org/2534333002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:42: ThreadUtils.runOnUiThreadBlocking(new Runnable() { On 2016/12/01 16:21:22, Theresa Wellington wrote: > nit: Can this use the new helper method simulateRendererKilled() too? (same for > testSadTabNotShownWhen... as well) Yes it can.
jwanda@chromium.org changed reviewers: + holte@chromium.org
holte: Please review changes in tools/metrics/actions/actions.xml I added a metric to differentiate when the user sends feedback via the menu or after a successive crash from a sad tab. When you have the time, can you look over this?
lgtm % nits https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1733: * @param recordAction The user action to record nit: add a period to the end of this sentence https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1882: OnClickListener reloadButtonAction = new OnClickListener() { nit: since this may not be a reload anymore, the variable should be renamed -- maybe just buttonAction. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:60: * Confirm that after a successive refresh of a failed tab we change the button from "Reload" nit: ... on a tab that failed to load.... https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:78: assertEquals(getActivity().getString(R.string.sad_tab_send_feedback_label), actualText); nit: Should this have a description too, eg.. "Expected the sad tab button to have the feedback label" https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:83: * successful, and then we crash again, we do not show the "Send Feedback" git cl format messed up the wrapping again https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:84: * button and instead show the "Reload" tab nit: period at the end of this sentence https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:110: super nit: remove this blank line https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:119: * Helper method that reloads a tab with a SadTabView currently displayed nit: period at the end of this sentence and on the description/params for the method below
Fixed everything based off your messages! Thanks again! Just gotta wait for 2 more lgtms and I'll be good to submit. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:1733: * @param recordAction The user action to record On 2016/12/01 19:54:05, Theresa Wellington wrote: > nit: add a period to the end of this sentence Done. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2534333002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1882: OnClickListener reloadButtonAction = new OnClickListener() { On 2016/12/01 19:54:05, Theresa Wellington wrote: > nit: since this may not be a reload anymore, the variable should be renamed -- > maybe just buttonAction. Done. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java (right): https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:60: * Confirm that after a successive refresh of a failed tab we change the button from "Reload" On 2016/12/01 19:54:05, Theresa Wellington wrote: > nit: ... on a tab that failed to load.... Done. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:78: assertEquals(getActivity().getString(R.string.sad_tab_send_feedback_label), actualText); On 2016/12/01 19:54:05, Theresa Wellington wrote: > nit: Should this have a description too, eg.. "Expected the sad tab button to > have the feedback label" Done. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:83: * successful, and then we crash again, we do not show the "Send Feedback" On 2016/12/01 19:54:05, Theresa Wellington wrote: > git cl format messed up the wrapping again It's really weird how this works. Mostly because I'm not using git cl format? I generally handle formatting errors on my own. Is it auto executed? https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:84: * button and instead show the "Reload" tab On 2016/12/01 19:54:05, Theresa Wellington wrote: > nit: period at the end of this sentence Done. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:110: On 2016/12/01 19:54:05, Theresa Wellington wrote: > super nit: remove this blank line Super done. https://codereview.chromium.org/2534333002/diff/60001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/tab/SadTabTest.java:119: * Helper method that reloads a tab with a SadTabView currently displayed On 2016/12/01 19:54:05, Theresa Wellington wrote: > nit: period at the end of this sentence and on the description/params for the > method below Added on my checklist before submit to add periods to all comments now.
OWNERS lgtm; Theresa seems to have this covered.
The CQ bit was checked by jwanda@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/12/01 22:55:08, dfalcantara (check my queue) wrote: > OWNERS lgtm; Theresa seems to have this covered. Thanks!
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jwanda@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sdefresne@chromium.org, twellington@chromium.org Link to the patchset: https://codereview.chromium.org/2534333002/#ps80001 (title: "Fixed code based off of twellington's comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480635786258710,
"parent_rev": "15525bd34330c993bff44969fd50e13c770b5f64", "commit_rev":
"5f451d7cc6d3bd803e3344c8788c0113fd700477"}
Message was sent while issue was closed.
Description was changed from ========== Show 'Send Feedback' on the sad tab page on multiple failures Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 ========== to ========== Show 'Send Feedback' on the sad tab page on multiple failures Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Show 'Send Feedback' on the sad tab page on multiple failures Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 ========== to ========== Show 'Send Feedback' on the sad tab page on multiple failures Fixed bug 665237 Fixed a bug where if you reloaded twice it would not show you "Send Feedback" as the primary button. As this is how it is in both all other Chrome versions we emulated this feature here as well. BUG=665237 Committed: https://crrev.com/e3f3136a52ce8425f1a67d470203f83f8e961245 Cr-Commit-Position: refs/heads/master@{#435774} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e3f3136a52ce8425f1a67d470203f83f8e961245 Cr-Commit-Position: refs/heads/master@{#435774} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
