Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2215063002:
Fix Print Preview Alt + Left Arrow breakage (Closed)
DescriptionFix Print Preview Alt + Left Arrow breakage
Fixing print preview closing in g-mail bug caused ALT + left
arrow to break for certain cases. Changed condition for not
closing dialog to be more specific.
BUG=636234, 634237
Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a
Committed: https://crrev.com/2224d2c0c1863c313fa3d4fb510091bc067c1d3e
Cr-Original-Commit-Position: refs/heads/master@{#410717}
Cr-Commit-Position: refs/heads/master@{#412697}
Patch Set 1 #
Total comments: 1
Patch Set 2 : fix indentation #Patch Set 3 : Add test #
Total comments: 26
Patch Set 4 : Fix comments and style issues #
Total comments: 2
Patch Set 5 : formatting #Patch Set 6 : Add extra nav check #Patch Set 7 : more nav checks #Patch Set 8 : add logging #Patch Set 9 : more logging #Patch Set 10 : Two possible tests to fix mac flakes #
Total comments: 5
Patch Set 11 : Use test 1. Revert controller changes. #Patch Set 12 : Use test 1, revert controller changes #
Messages
Total messages: 42 (22 generated)
Description was changed from ========== Fix ALT+ARROW break Fixing bug where print preview closed in g-mail caused ALT + left arrow to also break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 ========== to ========== Fix ALT+ARROW break Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 ==========
rbpotter@chromium.org changed reviewers: + thestig@chromium.org
The CQ bit was checked by rbpotter@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...
Description was changed from ========== Fix ALT+ARROW break Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 ========== to ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 ==========
Can we add a PrintPreviewDialogControllerUnitTest or maybe PrintPreviewDialogControllerBrowserTest to test for this? https://codereview.chromium.org/2215063002/diff/1/chrome/browser/printing/pri... File chrome/browser/printing/print_preview_dialog_controller.cc (right): https://codereview.chromium.org/2215063002/diff/1/chrome/browser/printing/pri... chrome/browser/printing/print_preview_dialog_controller.cc:354: (ui::PageTransitionTypeIncludingQualifiersIs(type, I'd run "git cl format" and let it sort out the indentations.
On 2016/08/04 20:58:24, Lei Zhang wrote: > Can we add a PrintPreviewDialogControllerUnitTest or maybe > PrintPreviewDialogControllerBrowserTest to test for this? Maybe with RenderViewHostTestHarness and RenderViewHostTestHarness::NavigateAndCommit() in particular, the test won't even need to set up an EmbeddedTestServer.
On 2016/08/04 21:14:40, Lei Zhang wrote: > On 2016/08/04 20:58:24, Lei Zhang wrote: > > Can we add a PrintPreviewDialogControllerUnitTest or maybe > > PrintPreviewDialogControllerBrowserTest to test for this? > > Maybe with RenderViewHostTestHarness and > RenderViewHostTestHarness::NavigateAndCommit() in particular, the test won't > even need to set up an EmbeddedTestServer. I'm going to look at expanding the test in line 222 of print_preview_dialog_controller_browsertest.cc. Looks like it is basically what we want to do for the test of destroying the dialog for navigating "back" to md-settings, and would just need to test the opposite for gmail with a couple of slightly different URLs.
Finally finished writing the test. Can add more navigation cases if desired, this covers the two that prompted the bugs.
https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_unittest.cc (right): https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:179: No blank lines at the beginning and end of functions. (git cl lint should tell you this) https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:180: // Two similar URLs (same webpage, slight difference at end) s/slight difference.*/different URL fragments/ https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:192: content::NavigationController *controller = content::NavigationController* controller = <-- star on the left but why just just take a reference since that's what GetController() returns? https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:192: content::NavigationController *controller = Name this nav_controller since there's also a dialog controller? https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:194: ASSERT_TRUE(controller); This cannot ever fail. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:196: // Get Print preview controller and navigate to first page Do you have to do it in this order? |dialog_controller| isn't used until slightly later. Can we declare variables closer to where they are used? https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:205: PrintViewManager *manager = PrintViewManager::FromWebContents(web_contents); PrintViewManager* manager... https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:207: WebContents* preview_dialog = Maybe tiger_preview_dialog (and tiger_barb_preview_dialog, etc below) instead of _2, _3, etc. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:221: // Print preview now should return true as dialog should be closed. "Print preview should now return true because the navigation should have closed |tiger_preview_dialog|." https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:222: ASSERT_TRUE(manager->PrintPreviewNow(false)); Can you do another PrintPreviewNow() before the "tiger_barb" navigation and check that fails? The comment on the previous line seems to indicate that should be the case. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:222: ASSERT_TRUE(manager->PrintPreviewNow(false)); Do the ASSERT.*()s here and below need to be ASSERT_, or can they be EXPECT_ ? https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:228: EXPECT_NE(preview_dialog, preview_dialog_2); Also check |preview_dialog_2| is not |web_contents| https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:230: // Navigate with back button or CTRL+LEFT ARROW to a similar page. Did you mean ALT?
https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_unittest.cc (right): https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:179: On 2016/08/09 01:20:16, Lei Zhang wrote: > No blank lines at the beginning and end of functions. (git cl lint should tell > you this) Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:180: // Two similar URLs (same webpage, slight difference at end) On 2016/08/09 01:20:17, Lei Zhang wrote: > s/slight difference.*/different URL fragments/ Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:192: content::NavigationController *controller = On 2016/08/09 01:20:17, Lei Zhang wrote: > Name this nav_controller since there's also a dialog controller? Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:192: content::NavigationController *controller = On 2016/08/09 01:20:16, Lei Zhang wrote: > content::NavigationController* controller = <-- star on the left > > but why just just take a reference since that's what GetController() returns? Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:194: ASSERT_TRUE(controller); On 2016/08/09 01:20:17, Lei Zhang wrote: > This cannot ever fail. Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:196: // Get Print preview controller and navigate to first page On 2016/08/09 01:20:16, Lei Zhang wrote: > Do you have to do it in this order? |dialog_controller| isn't used until > slightly later. Can we declare variables closer to where they are used? Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:205: PrintViewManager *manager = PrintViewManager::FromWebContents(web_contents); On 2016/08/09 01:20:16, Lei Zhang wrote: > PrintViewManager* manager... Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:207: WebContents* preview_dialog = On 2016/08/09 01:20:16, Lei Zhang wrote: > Maybe tiger_preview_dialog (and tiger_barb_preview_dialog, etc below) instead of > _2, _3, etc. Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:221: // Print preview now should return true as dialog should be closed. On 2016/08/09 01:20:17, Lei Zhang wrote: > "Print preview should now return true because the navigation should have closed > |tiger_preview_dialog|." Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:222: ASSERT_TRUE(manager->PrintPreviewNow(false)); On 2016/08/09 01:20:16, Lei Zhang wrote: > Do the ASSERT.*()s here and below need to be ASSERT_, or can they be EXPECT_ ? Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:222: ASSERT_TRUE(manager->PrintPreviewNow(false)); On 2016/08/09 01:20:16, Lei Zhang wrote: > Do the ASSERT.*()s here and below need to be ASSERT_, or can they be EXPECT_ ? Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:228: EXPECT_NE(preview_dialog, preview_dialog_2); On 2016/08/09 01:20:17, Lei Zhang wrote: > Also check |preview_dialog_2| is not |web_contents| Done. https://codereview.chromium.org/2215063002/diff/40001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:230: // Navigate with back button or CTRL+LEFT ARROW to a similar page. On 2016/08/09 01:20:16, Lei Zhang wrote: > Did you mean ALT? Done.
The CQ bit was checked by rbpotter@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/2215063002/diff/60001/chrome/browser/printing... File chrome/browser/printing/print_preview_dialog_controller_unittest.cc (right): https://codereview.chromium.org/2215063002/diff/60001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:192: web_contents->GetController(); Fits on previous line. git cl format is your friend. https://codereview.chromium.org/2215063002/diff/60001/chrome/browser/printing... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:196: ui::PageTransitionFromInt(ui::PAGE_TRANSITION_LINK), indentation is off. git cl format...
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2215063002/#ps80001 (title: "formatting")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 ========== to ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 ========== to ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/2232853002/ by foolip@chromium.org. The reason for reverting is: PrintPreviewDialogControllerUnitTest.CloseDialogOnNavigation is flaky on mac_chromium_rel_ng: PrintPreviewDialogControllerUnitTest.CloseDialogOnNavigation (run #1): [ RUN ] PrintPreviewDialogControllerUnitTest.CloseDialogOnNavigation [41473:1287:0810/034931:18891494657941:ERROR:native_widget_mac.mm(285)] Not implemented reached in virtual void views::NativeWidgetMac::SetWindowIcons(const gfx::ImageSkia &, const gfx::ImageSkia &) ../../chrome/browser/printing/print_preview_dialog_controller_unittest.cc:240: Failure Expected: (tiger_barb_preview_dialog) != (tiger_preview_dialog_2), actual: 0x7f9efc809a00 vs 0x7f9efc809a00 [ FAILED ] PrintPreviewDialogControllerUnitTest.CloseDialogOnNavigation (89 ms) BUG=636234.
Message was sent while issue was closed.
Description was changed from ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ========== to ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ==========
Lei, Have 2 possible tests right now. One uses a new subclass of the print preview dialog controller and the other one uses either destruction or non-equivalent pointers as the check that preview dialogs aren't identical. Let me know if these work on Mac (I think they should, but not 100% certain until we test it) and which you would prefer to add. Thanks, Rebekah
Most of the comments probably won't be applicable if we go with the "V1" test. https://codereview.chromium.org/2215063002/diff/180001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller.h (right): https://codereview.chromium.org/2215063002/diff/180001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.h:83: // Moved to this category so that testing subclass works No need for the comment in the code to check in. Maybe just write a comment in code review instead next time? https://codereview.chromium.org/2215063002/diff/180001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller.h:93: PrintPreviewDialogMap preview_dialog_map_; Can we provide a setter method instead? Keep the member variable private. https://codereview.chromium.org/2215063002/diff/180001/chrome/browser/printin... File chrome/browser/printing/print_preview_dialog_controller_unittest.cc (right): https://codereview.chromium.org/2215063002/diff/180001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:53: next_dialog_ = 1; Use the initializer list https://codereview.chromium.org/2215063002/diff/180001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:61: if (!preview_dialog) { I usually like to write: if (foo) { DoFoo(); } else { DoBar(); } rather than if (!foo) ... https://codereview.chromium.org/2215063002/diff/180001/chrome/browser/printin... chrome/browser/printing/print_preview_dialog_controller_unittest.cc:242: TEST_F(PrintPreviewDialogControllerUnitTest, CloseDialogOnNavigation) { I prefer this version mainly because we don't have to modify the controller.
The CQ bit was checked by rbpotter@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...
Lei, Changed to use test 1 only and rolled back the changes to the dialog controller. Bots are running now. -Rebekah
lgtm
Description was changed from ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ========== to ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=636234, 634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ==========
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 rbpotter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=636234, 634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ========== to ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=636234, 634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=636234, 634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Cr-Commit-Position: refs/heads/master@{#410717} ========== to ========== Fix Print Preview Alt + Left Arrow breakage Fixing print preview closing in g-mail bug caused ALT + left arrow to break for certain cases. Changed condition for not closing dialog to be more specific. BUG=636234, 634237 Committed: https://crrev.com/d65b6608c1349ba70606565b2bed9ff8b016473a Committed: https://crrev.com/2224d2c0c1863c313fa3d4fb510091bc067c1d3e Cr-Original-Commit-Position: refs/heads/master@{#410717} Cr-Commit-Position: refs/heads/master@{#412697} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/2224d2c0c1863c313fa3d4fb510091bc067c1d3e Cr-Commit-Position: refs/heads/master@{#412697} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
