Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
26678004:
Added a browser test of the print preview dialog size for Chrome apps. (Closed)
|
Created:
7 years, 2 months ago by dharcourt Modified:
7 years, 2 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org, miket_OOO Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdded a browser test of the print preview dialog size for Chrome apps.
The added PrintPreviewShouldNotBeTooSmall test verifies that the print
preview dialog is big enough to be usable even when printing small
application windows. Because the print preview dialog is currently
limited to the size of the window being printed on all platforms but
OS X, this test currently fails on all platforms but OS X.
BUG=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=228747
Patch Set 1 #
Total comments: 8
Patch Set 2 : Refactored to use WebContentsView::GetContainerSize instead of JavaScript's window.onresize. #
Total comments: 13
Patch Set 3 : Implemented review suggestions & rebased #
Total comments: 10
Patch Set 4 : Implemented follow-up review suggestions #
Total comments: 2
Patch Set 5 : Implemented follow-up review suggestions #
Messages
Total messages: 16 (0 generated)
Hi Vitaly,
This is a tentative implementation of a preview dialog size test for Chrome
apps. Please let me know if you think I should do this another way, I'm open to
suggestions.
Thanks!
- Charles
https://codereview.chromium.org/26678004/diff/1/chrome/browser/apps/app_brows... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/1/chrome/browser/apps/app_brows... chrome/browser/apps/app_browsertest.cc:1133: EXPECT_GE(size_monitor.GetSizeForTesting().width(), 200); I guess 200 is aprox size, so why do you need size from javascript instead of just getting, let say WebContentsView::GetContainerSize https://codereview.chromium.org/26678004/diff/1/chrome/browser/resources/prin... File chrome/browser/resources/print_preview/print_preview.js (right): https://codereview.chromium.org/26678004/diff/1/chrome/browser/resources/prin... chrome/browser/resources/print_preview/print_preview.js:887: onWindowResized_: function() { Looks like this one is the first without doc string https://codereview.chromium.org/26678004/diff/1/chrome/browser/ui/webui/print... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/26678004/diff/1/chrome/browser/ui/webui/print... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:365: // This is a pointer to avoid an exit-time destructor. Comment should be above var https://codereview.chromium.org/26678004/diff/1/chrome/browser/ui/webui/print... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:642: void PrintPreviewUI::ScopedSizeMonitorForTesting::RecordSizeForTesting( If you can avoid changing print_preview_handler.cc this static is not needed and size can be calculated here.
Vitaly, you may want to hold off on reviewing this. I'm going to replace the
ugly timed sleeps in the test with observers, so things will change. I'd be
happy to integrate any comments you have if you already looked at this, of
course.
Sorry for the false start, more in a bit.
- Charles
Hi Vitaly, a candidate-for-committing patch has been uploaded, please review at
your convenience. The code has changed enough that you may want to ignore
previous patches and just review this final patch as if it were a new change
list. Thanks for your comments on the earlier version.
Antony, could you OWNERS review app_browsertest.cc and test.js?
Thanks!
- Charles
https://codereview.chromium.org/26678004/diff/1/chrome/browser/apps/app_brows...
File chrome/browser/apps/app_browsertest.cc (right):
https://codereview.chromium.org/26678004/diff/1/chrome/browser/apps/app_brows...
chrome/browser/apps/app_browsertest.cc:1133:
EXPECT_GE(size_monitor.GetSizeForTesting().width(), 200);
On 2013/10/09 20:57:37, Vitaly Buka wrote:
> I guess 200 is aprox size, so why do you need size from javascript instead of
> just getting, let say WebContentsView::GetContainerSize
Initially I thought I would need to keep track of the dialog size as it changed.
It turns out I don't and for this test I can use a snapshot of the size of the
dialog when it's ready, so I've changed the code to use
WebContentsView::GetContainerSize and removed the JavaScript part. Thanks for
the suggestion.
https://codereview.chromium.org/26678004/diff/1/chrome/browser/resources/prin...
File chrome/browser/resources/print_preview/print_preview.js (right):
https://codereview.chromium.org/26678004/diff/1/chrome/browser/resources/prin...
chrome/browser/resources/print_preview/print_preview.js:887: onWindowResized_:
function() {
On 2013/10/09 20:57:37, Vitaly Buka wrote:
> Looks like this one is the first without doc string
Done. This function isn't needed any more. Resolution by elimination :-).
https://codereview.chromium.org/26678004/diff/1/chrome/browser/ui/webui/print...
File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right):
https://codereview.chromium.org/26678004/diff/1/chrome/browser/ui/webui/print...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:365: // This is a
pointer to avoid an exit-time destructor.
On 2013/10/09 20:57:37, Vitaly Buka wrote:
> Comment should be above var
Done. The variable and comment don't exist any more, and I'll be sure to put
comments above in the future.
https://codereview.chromium.org/26678004/diff/1/chrome/browser/ui/webui/print...
chrome/browser/ui/webui/print_preview/print_preview_ui.cc:642: void
PrintPreviewUI::ScopedSizeMonitorForTesting::RecordSizeForTesting(
On 2013/10/09 20:57:37, Vitaly Buka wrote:
> If you can avoid changing print_preview_handler.cc this static is not needed
and
> size can be calculated here.
Done.
https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br...
File chrome/browser/apps/app_browsertest.cc (right):
https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br...
chrome/browser/apps/app_browsertest.cc:129:
PrintPreviewUI::SetDelegateForTesting(this);
See print_preview_ui.h for comments on the SetTestingDelegate()
->SetDelegateForTesting() name change.
https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br...
chrome/browser/apps/app_browsertest.cc:154: }
It turns out that PreviewIsReady() was being called on every page render (i.e. 3
times for a 3 page document), causing the test verification to happen after the
first page is rendered. Page count logic was added to ensure testing continues
only after all pages are rendered.
https://codereview.chromium.org/26678004/diff/8001/chrome/browser/ui/webui/pr...
File chrome/browser/ui/webui/print_preview/print_preview_ui.h (right):
https://codereview.chromium.org/26678004/diff/8001/chrome/browser/ui/webui/pr...
chrome/browser/ui/webui/print_preview/print_preview_ui.h:160: static void
SetDelegateForTesting(TestingDelegate* delegate);
If this method's name ends in "ForTesting" the presubmit hooks will verify that
it is being called only from test code, which is what we want, so I changed the
name. To avoid unnecessary submit warnings I applied the reverse change to other
methods whose names previously ended in "ForTesting" but that were actually
designed to be called from non-testing code (even if called for testing
purposes).
lgtm https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:127: waiting_for_ready_(false), can you remove waiting_for_ready_ and just use if (waiting_runner_) checks? https://codereview.chromium.org/26678004/diff/8001/chrome/browser/ui/webui/pr... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/26678004/diff/8001/chrome/browser/ui/webui/pr... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:348: void NotifyTestingDelegateOfPreviewPageCount(int page_count) { I'd prefer to remove this functions and just put logic in-place.
lgtm https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:148: OVERRIDE {; what's this ";" doing here? :) https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:151: if (waiting_for_ready_ && rendered_count_ >= page_count_) { Is it actually valid for rendered_count_ to be greater than page_count_? If not, maybe do this instead: CHECK(rendered_count_ <= page_count_); if (waiting_for_ready_ && rendered_count_ == page_count_) { ... https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:171: int rendered_count_; nit: these two new variables would benefit from a brief comment describing their meaning. Is page_count_ the total number of pages we expect to see? etc.
https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:127: waiting_for_ready_(false), On 2013/10/14 21:13:55, Vitaly Buka wrote: > can you remove waiting_for_ready_ and just use if (waiting_runner_) checks? Done. https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:148: OVERRIDE {; On 2013/10/14 23:02:59, Antony Sargent wrote: > what's this ";" doing here? :) Purely ornamental. In other words, typo. Removed now, thanks for the sharp eyes. https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:151: if (waiting_for_ready_ && rendered_count_ >= page_count_) { On 2013/10/14 23:02:59, Antony Sargent wrote: > Is it actually valid for rendered_count_ to be greater than page_count_? If not, > maybe do this instead: > > CHECK(rendered_count_ <= page_count_); > if (waiting_for_ready_ && rendered_count_ == page_count_) { > ... > Done. https://codereview.chromium.org/26678004/diff/8001/chrome/browser/apps/app_br... chrome/browser/apps/app_browsertest.cc:171: int rendered_count_; On 2013/10/14 23:02:59, Antony Sargent wrote: > nit: these two new variables would benefit from a brief comment describing their > meaning. Is page_count_ the total number of pages we expect to see? etc. Done. I renamed page_count_ to total_page_count_ and rendered_count_ to rendered_page_count_. Hopefully this will be even clearer than a comment. https://codereview.chromium.org/26678004/diff/8001/chrome/browser/ui/webui/pr... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/26678004/diff/8001/chrome/browser/ui/webui/pr... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:348: void NotifyTestingDelegateOfPreviewPageCount(int page_count) { On 2013/10/14 21:13:55, Vitaly Buka wrote: > I'd prefer to remove this functions and just put logic in-place. Done. I assumed you'd want the same done with NotifyTestingDelegateOfPreviewPageRendering, let me know if that's not the case and I'll fix it in a follow-up change list.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dharcourt@chromium.org/26678004/16001
https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:158: waiting_runner_->Run(); Now waiting_runner_ is NULL always? https://codereview.chromium.org/26678004/diff/16001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/26678004/diff/16001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:343: bool IsAutoCancelForTestingEnabled() { Why do you still keep this one?
https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:158: waiting_runner_->Run(); On 2013/10/15 01:09:11, Vitaly Buka wrote: > Now waiting_runner_ is NULL always? waiting_runner_ is NULL in the testing context outside of WaitUntilPreviewIsReady() but it is non-null on the PrintPreviewUI context that calls DidRenderPreviewPage(). I'm assuming these are running on different threads of the browser process, with the WaitUntilPreviewIsReady() thread being the original browser thread and the DidRenderPreviewPage() thread being the thread created when a new content::MessageLoopRunner was created. Correct? I can't NULL out waiting_runner_ anywhere else, particularly not in DidRenderPreviewPage, because then the waiting_runner_ runner will be deleted while still in the waiting_runner_->Run() call, causing a segfault. I tried :-). https://codereview.chromium.org/26678004/diff/16001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/26678004/diff/16001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:343: bool IsAutoCancelForTestingEnabled() { On 2013/10/15 01:09:11, Vitaly Buka wrote: > Why do you still keep this one? I hadn't thought of removing it, but it does make sense. Removed now.
https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:150: if (waiting_runner_ && rendered_page_count_ == total_page_count_) { Why do you need to check waiting_runner I guess it's called always from WaitUntilPreviewIsReady? Can you quit current message loop? https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:156: if (!waiting_runner_ && rendered_page_count_ < total_page_count_) { You don't need to check waiting_runner_ Maybe just DCHECK(!waiting_runner_) https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:158: waiting_runner_->Run(); Oh, I see now. Maybe can also remove waiting_runner_? void WaitUntilPreviewIsReady() { if (rendered_page_count_ < total_page_count_) { scope_refptr<> waiting_runner = new content::MessageLoopRunner; waiting_runner->Run(); } } On 2013/10/15 01:47:49, dharcourt wrote: > On 2013/10/15 01:09:11, Vitaly Buka wrote: > > Now waiting_runner_ is NULL always? > > waiting_runner_ is NULL in the testing context outside of > WaitUntilPreviewIsReady() but it is non-null on the PrintPreviewUI context that > calls DidRenderPreviewPage(). > > I'm assuming these are running on different threads of the browser process, with > the WaitUntilPreviewIsReady() thread being the original browser thread and the > DidRenderPreviewPage() thread being the thread created when a new > content::MessageLoopRunner was created. Correct? > > I can't NULL out waiting_runner_ anywhere else, particularly not in > DidRenderPreviewPage, because then the waiting_runner_ runner will be deleted > while still in the waiting_runner_->Run() call, causing a segfault. I tried :-).
Still lgtm https://codereview.chromium.org/26678004/diff/24001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/26678004/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:506: if (g_testing_delegate_ != NULL && g_testing_delegate_->IsAutoCancelEnabled()) I believe this file, and maybe Chromium in general use mostly if (g_testing_delegate_) check style
https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... File chrome/browser/apps/app_browsertest.cc (right): https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:150: if (waiting_runner_ && rendered_page_count_ == total_page_count_) { On 2013/10/15 02:18:06, Vitaly Buka wrote: > Why do you need to check waiting_runner > I guess it's called always from WaitUntilPreviewIsReady? > Can you quit current message loop? Currently WaitUntilPreviewIsReady() is called immediately after the print preview creation that will cause DidRenderPreviewPage() to be called. If this changes in the future DidRenderPreviewPage() may be called before WaitUntilPreviewIsReady (maybe something will be added that causes events to be processed befre WaitUntilPrevieweIsReady). This check ensures that this delegate will continue to work in that situation, and generally decouples it from how its used. https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:156: if (!waiting_runner_ && rendered_page_count_ < total_page_count_) { On 2013/10/15 02:18:06, Vitaly Buka wrote: > You don't need to check waiting_runner_ > Maybe just DCHECK(!waiting_runner_) Done. I think I can actually use CHECK() as Antony suggested I do for DidRenderPreviewPage because this is a test. https://codereview.chromium.org/26678004/diff/16001/chrome/browser/apps/app_b... chrome/browser/apps/app_browsertest.cc:158: waiting_runner_->Run(); On 2013/10/15 02:18:06, Vitaly Buka wrote: > Oh, I see now. > > Maybe can also remove waiting_runner_? > > void WaitUntilPreviewIsReady() { > if (rendered_page_count_ < total_page_count_) { > scope_refptr<> waiting_runner = new content::MessageLoopRunner; > waiting_runner->Run(); > } > } > On 2013/10/15 01:47:49, dharcourt wrote: > > On 2013/10/15 01:09:11, Vitaly Buka wrote: > > > Now waiting_runner_ is NULL always? > > > > waiting_runner_ is NULL in the testing context outside of > > WaitUntilPreviewIsReady() but it is non-null on the PrintPreviewUI context > that > > calls DidRenderPreviewPage(). > > > > I'm assuming these are running on different threads of the browser process, > with > > the WaitUntilPreviewIsReady() thread being the original browser thread and the > > DidRenderPreviewPage() thread being the thread created when a new > > content::MessageLoopRunner was created. Correct? > > > > I can't NULL out waiting_runner_ anywhere else, particularly not in > > DidRenderPreviewPage, because then the waiting_runner_ runner will be deleted > > while still in the waiting_runner_->Run() call, causing a segfault. I tried > :-). > I don't think I can remove waiting_runner_ because I need it in DidRenderPreviewPage(). https://codereview.chromium.org/26678004/diff/24001/chrome/browser/ui/webui/p... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/26678004/diff/24001/chrome/browser/ui/webui/p... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:506: if (g_testing_delegate_ != NULL && g_testing_delegate_->IsAutoCancelEnabled()) On 2013/10/15 02:20:12, Vitaly Buka wrote: > I believe this file, and maybe Chromium in general use mostly > if (g_testing_delegate_) check style Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dharcourt@chromium.org/26678004/41001
Message was sent while issue was closed.
Change committed as 228747 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
