Browser Plugin: Implement CanGoBack/CanGoForward

Browser Plugin: Implement CanGoBack/CanGoForward

BUG=154831
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=161414

[Fady Samuel, 2012-10-09 17:10:18]

Hi Charlie,

This seemed like some low-hanging fruit to me, so I decided to get it out of the
way. :-)

Fady

[Fady Samuel, 2012-10-09 17:10:54]

Hi Charlie,

This seemed like some low-hanging fruit to me, so I decided to get it out of the
way. :-)

Fady

[Charlie Reis, 2012-10-09 18:46:14]

On 2012/10/09 17:10:54, Fady Samuel wrote:
> Hi Charlie,
> 
> This seemed like some low-hanging fruit to me, so I decided to get it out of
the
> way. :-)
> 
> Fady

Hmm, I want to spend some time on this one, actually, since introducing more
back/forward state scares me.  We had two insanely complex, 100+ comment bugs
due to the distributed navigation state in Chrome (http://crbug.com/93427 and
http://crbug.com/89798).  I want to be sure we understand this thoroughly before
adding more state.

[Charlie Reis, 2012-10-10 06:06:53]

Some thoughts inline.  The main thing that bothers me is that this is a slippery
slope towards having a history API in the embedder (e.g., what if a user clicks
and holds on the back button?  Should a list of entries be visible?).  That gets
*incredibly* hard to do correctly across processes, without having URL spoofs in
the app.

That's one reason I want to be very careful about what API we expose here.

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:389:
web_contents()->GetController().GetEntryCount()));
I think we should combine these two IPC messages into one.  (It should be sent
on all navigations, and is_main_frame should just be a parameter.)  Note that
Robert has a CL in progress for sending is_main_frame as a parameter here:
https://codereview.chromium.org/11035067/

Also, I'm 90% sure that commits are the only way that index and count will
change, but if I'm forgetting something, then the app will be confused about
when it can go back and forward.  (Things like prerendering, instant, session
restore, pushState, and others have all surprised me in the past.)

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:533:
EXPECT_EQ(expected_title, actual_title);
Probably worth checking that canGoForward is false and canGoBack is true, before
we start using back and forward.

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:547:
EXPECT_TRUE(result);
I think it would be clearer to move this check after the Back(), and also check
that CanGoForward() is true.  (Otherwise it reads strangely because the comment
above makes it sound like the back navigation has already happened.)

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:583:
EXPECT_TRUE(result);
Again, we should be testing cases when CanGoBack returns false (i.e., after the
Go(-2)).

https://codereview.chromium.org/11092023/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11092023/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:62: void
NavigationStateUpdate(int current_entry_index, int entry_count);
Seems like this can just be merged into DidNavigate.

https://codereview.chromium.org/11092023/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:210: int nav_entry_count_;
I get really scared by keeping this state here, but I don't see another way to
implement it without having an asynchronous reply.  We need to avoid having this
grow into more history state that could lead to broken navigation in apps,
though.  Maybe it's overkill, but I suggest the following warning comment above
these two fields:

Important: Do not add more history state here.
We strongly discourage storing additional history state (such as page IDs) in
the embedder process, at the risk of having incorrect information that can lead
to broken back/forward logic in apps.
It's also important that this state does not get modified by any logic in the
embedder process.  It should only be updated in response to navigation events in
the guest.  No assumptions should be made about how the index will change after
a navigation (e.g., for back, forward, or go), because the changes are not
always obvious.  For example, there is a maximum number of entries and earlier
ones will automatically be pruned.

[Fady Samuel, 2012-10-10 17:36:44]

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_guest.cc:389:
web_contents()->GetController().GetEntryCount()));
On 2012/10/10 06:06:53, creis wrote:
> I think we should combine these two IPC messages into one.  (It should be sent
> on all navigations, and is_main_frame should just be a parameter.)  Note that
> Robert has a CL in progress for sending is_main_frame as a parameter here:
> https://codereview.chromium.org/11035067/
> 
> Also, I'm 90% sure that commits are the only way that index and count will
> change, but if I'm forgetting something, then the app will be confused about
> when it can go back and forward.  (Things like prerendering, instant, session
> restore, pushState, and others have all surprised me in the past.)

Done.

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:547:
EXPECT_TRUE(result);
On 2012/10/10 06:06:53, creis wrote:
> I think it would be clearer to move this check after the Back(), and also
check
> that CanGoForward() is true.  (Otherwise it reads strangely because the
comment
> above makes it sound like the back navigation has already happened.)

Done.

https://codereview.chromium.org/11092023/diff/1/content/browser/browser_plugi...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:583:
EXPECT_TRUE(result);
On 2012/10/10 06:06:53, creis wrote:
> Again, we should be testing cases when CanGoBack returns false (i.e., after
the
> Go(-2)).

Done.

https://codereview.chromium.org/11092023/diff/1/content/renderer/browser_plug...
File content/renderer/browser_plugin/browser_plugin.h (right):

https://codereview.chromium.org/11092023/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:62: void
NavigationStateUpdate(int current_entry_index, int entry_count);
On 2012/10/10 06:06:53, creis wrote:
> Seems like this can just be merged into DidNavigate.

Done.

https://codereview.chromium.org/11092023/diff/1/content/renderer/browser_plug...
content/renderer/browser_plugin/browser_plugin.h:210: int nav_entry_count_;
On 2012/10/10 06:06:53, creis wrote:
> I get really scared by keeping this state here, but I don't see another way to
> implement it without having an asynchronous reply.  We need to avoid having
this
> grow into more history state that could lead to broken navigation in apps,
> though.  Maybe it's overkill, but I suggest the following warning comment
above
> these two fields:
> 
> Important: Do not add more history state here.
> We strongly discourage storing additional history state (such as page IDs) in
> the embedder process, at the risk of having incorrect information that can
lead
> to broken back/forward logic in apps.
> It's also important that this state does not get modified by any logic in the
> embedder process.  It should only be updated in response to navigation events
in
> the guest.  No assumptions should be made about how the index will change
after
> a navigation (e.g., for back, forward, or go), because the changes are not
> always obvious.  For example, there is a maximum number of entries and earlier
> ones will automatically be pruned.

Done.

[Charlie Reis, 2012-10-11 19:03:01]

Hmm, ok, LGTM with the comment below.  Please check with me before adding any
other history stuff, though.  :)

http://codereview.chromium.org/11092023/diff/8002/content/browser/browser_plu...
File content/browser/browser_plugin/browser_plugin_host_browsertest.cc (right):

http://codereview.chromium.org/11092023/diff/8002/content/browser/browser_plu...
content/browser/browser_plugin/browser_plugin_host_browsertest.cc:572:
EXPECT_TRUE(result);
Please move this after the Forward() call and have it EXPECT_FALSE instead.  (We
just checked if it was true above on line 558.)

[Fady Samuel, 2012-10-11 19:45:02]

On 2012/10/11 19:03:01, creis wrote:
> Hmm, ok, LGTM with the comment below.  Please check with me before adding any
> other history stuff, though.  :)

Fixed. Of course all API changes will go through you. :-)

[commit-bot: I haz the power, 2012-10-11 19:46:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/fsamuel@chromium.org/11092023/10012

[commit-bot: I haz the power, 2012-10-11 21:50:08]

Change committed as 161414

[Mihai Parparita -not on Chrome, 2012-10-16 20:35:16]

On 2012/10/11 21:50:08, I haz the power (commit-bot) wrote:
> Change committed as 161414

Should these methods be exposed in the <browser> shim too?

[Fady Samuel, 2012-10-16 20:38:06]

On 2012/10/16 20:35:16, Mihai Parparita wrote:
> On 2012/10/11 21:50:08, I haz the power (commit-bot) wrote:
> > Change committed as 161414
> 
> Should these methods be exposed in the <browser> shim too?

Yes. Will create a new CL to expose these.