Could you take a look at this change since maruel is on vacation?  I also
touched some of your Mock*Files (simplification mainly).
The original change is very simple but I spent a lot of time trying to get a
subframe to load in the renderview_unittest without success and hence the
changes to the test and surroundings.

This seems like it could result in very confusing printer output in some cases. 
Imagine a page with a small iframe.  Suppose the user had given that iframe
focus unintentionally.  Now, they hit Ctrl+P.  With this change, we'd just print
that small iframe, right?  It seems like it may be unexpected behavior.  Perhaps
you should only do this if there is a text selection present?

These are drive-through nits.
Changing the MockPrinter class itself is OK for me. Nevertheless, I recommend
you to discuss with maruel whether or not this feature is useful for users.

http://codereview.chromium.org/149644/diff/1/6
File chrome/renderer/render_view_unittest.cc (right):

http://codereview.chromium.org/149644/diff/1/6#newcode358
Line 358: #if defined(OS_WIN)
nit: the MockNetwork class, which emulates our network stack for RenderViewTest,
is currently under development. Sorry for your inconvenience. :)

http://codereview.chromium.org/149644/diff/1/6#newcode383
Line 383: // Verify the page count is correct.
nit: This message has been handled by MockPrinter::SetPrintedPagesCount() and
its post_page_count_param.b is stored in |MockPrinter::number_pages_|. So, I
prefer adding a function that retrieves the |MockPrinter::number_pages_| value
to the MockPrinter class to hide this sort of low-level operations from unit
tests. (This is a reason why I added the MockPrinter class.)

http://codereview.chromium.org/149644/diff/1/6#newcode393
Line 393: // Verify the rendered "printed page".
nit: as noted above, this message has been handled by MockPrinter::PrintPage().
So, I prefer adding a function to the MockPrinter class that retrieves the
|page_number| value.

I haven't looked at the code yet,
The feature is useful and should be implemented but I agree maybe not by
default. At least the javascript wiring should be implemented (we currently
always print the whole page)

On Jul 14, 2009 11:10 PM, <hbono@chromium.org> wrote:

These are drive-through nits.
Changing the MockPrinter class itself is OK for me. Nevertheless, I
recommend you to discuss with maruel whether or not this feature is
useful for users.


http://codereview.chromium.org/149644/diff/1/6
File chrome/renderer/render_view_unittest.cc (right):

http://codereview.chromium.org/149644/diff/1/6#newcode358
Line 358: #if defined(OS_WIN)
nit: the MockNetwork class, which emulates our network stack for
RenderViewTest, is currently under development. Sorry for your
inconvenience. :)

http://codereview.chromium.org/149644/diff/1/6#newcode383
Line 383: // Verify the page count is correct.
nit: This message has been handled by
MockPrinter::SetPrintedPagesCount() and its post_page_count_param.b is
stored in |MockPrinter::number_pages_|. So, I prefer adding a function
that retrieves the |MockPrinter::number_pages_| value to the MockPrinter
class to hide this sort of low-level operations from unit tests. (This
is a reason why I added the MockPrinter class.)

http://codereview.chromium.org/149644/diff/1/6#newcode393
Line 393: // Verify the rendered "printed page".
nit: as noted above, this message has been handled by
MockPrinter::PrintPage(). So, I prefer adding a function to the
MockPrinter class that retrieves the |page_number| value.

http://codereview.chromium.org/149644

Uploaded a new version.  I agree that the solution was not ideal but I thought
it would also cause confusion if the user would only print one frame if she has
selected some text (even if the user was not planning on printing only the
selection).  But I guess that causes less of a confusion than the alternative
(at least less likely to cause a confusion).
I changed the implementation to always print the full page when printing from
javascript and only printing the currently selected frame if text is selected so
that print selection works.
The worst part is that I can't really test this since I haven't figured out how
to load content into a subframe in the renderview tests.

Now with comments...

http://codereview.chromium.org/149644/diff/1/5
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/149644/diff/1/5#newcode451
Line 451: Print(webview()->GetFocusedFrame(), false);
I changed this to print only the focused frame if there is text selected.

http://codereview.chromium.org/149644/diff/1/6
File chrome/renderer/render_view_unittest.cc (right):

http://codereview.chromium.org/149644/diff/1/6#newcode358
Line 358: #if defined(OS_WIN)
np - would this allow me to enhance the test to download the content of a
subframe?

http://codereview.chromium.org/149644/diff/1/6#newcode383
Line 383: // Verify the page count is correct.
On 2009/07/15 03:09:54, hbono wrote:
> nit: This message has been handled by MockPrinter::SetPrintedPagesCount() and
> its post_page_count_param.b is stored in |MockPrinter::number_pages_|. So, I
> prefer adding a function that retrieves the |MockPrinter::number_pages_| value
> to the MockPrinter class to hide this sort of low-level operations from unit
> tests. (This is a reason why I added the MockPrinter class.)

Good idea - I'm guilty of just basing my test on the previous printing tests.

http://codereview.chromium.org/149644/diff/1/6#newcode393
Line 393: // Verify the rendered "printed page".
On 2009/07/15 03:09:54, hbono wrote:
> nit: as noted above, this message has been handled by
MockPrinter::PrintPage().
> So, I prefer adding a function to the MockPrinter class that retrieves the
> |page_number| value.

Done.

http://codereview.chromium.org/149644/diff/1027/39
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/149644/diff/1027/39#newcode2321
Line 2321: Print(webview()->GetMainFrame(), true);
this seems like it is introducing a potential web compat bug.  why was this
change made?  if i use window.print(), other browsers will print the frame
corresponding to window.  it seems like this could matter to some web apps that
use window.print().  how do we know that this change is a good thing?

neither the description of this CL nor the comment above provide the
justification for this change.  can you please enlighten me? :-)

http://codereview.chromium.org/149644/diff/1027/39
File chrome/renderer/render_view.cc (right):

http://codereview.chromium.org/149644/diff/1027/39#newcode2321
Line 2321: Print(webview()->GetMainFrame(), true);
On 2009/09/16 16:28:50, darin wrote:
> this seems like it is introducing a potential web compat bug.  why was this
> change made?  if i use window.print(), other browsers will print the frame
> corresponding to window.  it seems like this could matter to some web apps
that
> use window.print().  how do we know that this change is a good thing?
> 
> neither the description of this CL nor the comment above provide the
> justification for this change.  can you please enlighten me? :-)

That is interesting, I didn't realize that change. I'll create a separate
change.