Remove WebHistoryItem child usage

Remove WebHistoryItem child usage

HistoryEntry is now used to store the history tree.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=267274

[Nate Chapin, 2014-04-25 21:49:21]

https://codereview.chromium.org/248013003/diff/40001/content/renderer/context...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/context...
content/renderer/context_menu_params_builder.cc:52: params.frame_page_state =
state;
I'm not 100% this is safe, since this was serializing only part of the frame
state before, and now it's serializing the state for the whole page. I can't
think of why you'd want only part of the history tree, though.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_entry.cc:92:
entry_->unique_names_to_items_[item.target().utf8()] = this;
When a HistoryNode is created during deserialization, it doesn't have a
WebHistoryItem with a valid target, so when updating the item, update the
target.

[jam, 2014-04-28 15:13:30]

I'm not familiar wit the details of this code; Charlie or Brett can give a more
thorough review than me.

[Charlie Reis, 2014-04-28 16:29:00]

Looking pretty good.  I just have a few questions below, mainly about the
context menu case.

https://codereview.chromium.org/248013003/diff/40001/content/public/test/rend...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/248013003/diff/40001/content/public/test/rend...
content/public/test/render_view_test.h:37: class PageState;
nit: Alphabetize.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/context...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/context...
content/renderer/context_menu_params_builder.cc:52: params.frame_page_state =
state;
On 2014/04/25 21:49:21, Nate Chapin wrote:
> I'm not 100% this is safe, since this was serializing only part of the frame
> state before, and now it's serializing the state for the whole page. I can't
> think of why you'd want only part of the history tree, though.

I'm not sure what it's used for, so it's hard to say.  We're probably going to
have to break PageState down into FrameState eventually, though, since each
frame's state will be stored on a separate FrameNavigationEntry in the browser
process (and each frame may come from a different renderer process).

Looking closer, it appears to be used in Browser::ViewSourceForFrame, which
loads a subframe URL and state at the top-level of a new tab in view-source mode
(see browser.cc).  That's potentially a problem, since we'll now have the
PageState for the entire page rather than the subframe that we're loading in a
new tab.  (You might even be able to test this out manually, but I'm not sure
how it will manifest.)

I wonder if there's a way to get the PageState for just the frame from |data|.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_entry.cc:92:
entry_->unique_names_to_items_[item.target().utf8()] = this;
On 2014/04/25 21:49:21, Nate Chapin wrote:
> When a HistoryNode is created during deserialization, it doesn't have a
> WebHistoryItem with a valid target, so when updating the item, update the
> target.

I'm not sure I follow this, but it also sounds like it would be useful to add as
a comment in the code.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
File content/renderer/history_serialization.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_serialization.cc:83: void
RecursivelyGenerateFrameState(HistoryEntry::HistoryNode* node,
Just to be sure I'm clear why it's ok for this to return void rather than a
bool, it looks like the old version could never actually return false, right? 
(Same below.)

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_serialization.cc:176: int main_routing_id) {
Just want to be sure I understand the reason for passing in the main frame's
routing ID.  HistoryEntry still assumes it's for the top-level frame and
includes the entire tree, right?  We'll have to later break it down into
something frame-specific so that parts of the tree can live in other processes.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_serialization.cc:179: return 0;
NULL

https://codereview.chromium.org/248013003/diff/40001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2127: ContextMenuParams params =
ContextMenuParamsBuilder::Build(data, state);
Yeah, somehow we'll need to PageState for the frame from |data|. 
(Interestingly, we want the state for the whole subtree starting at that frame,
which will be tricky when we move to FrameState.  Maybe this is something the
browser process could manage...)

[Nate Chapin, 2014-04-28 17:31:53]

https://codereview.chromium.org/248013003/diff/40001/content/public/test/rend...
File content/public/test/render_view_test.h (right):

https://codereview.chromium.org/248013003/diff/40001/content/public/test/rend...
content/public/test/render_view_test.h:37: class PageState;
On 2014/04/28 16:29:00, Charlie Reis wrote:
> nit: Alphabetize.

Done.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/context...
File content/renderer/context_menu_params_builder.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/context...
content/renderer/context_menu_params_builder.cc:52: params.frame_page_state =
state;
On 2014/04/28 16:29:00, Charlie Reis wrote:
> On 2014/04/25 21:49:21, Nate Chapin wrote:
> > I'm not 100% this is safe, since this was serializing only part of the frame
> > state before, and now it's serializing the state for the whole page. I can't
> > think of why you'd want only part of the history tree, though.
> 
> I'm not sure what it's used for, so it's hard to say.  We're probably going to
> have to break PageState down into FrameState eventually, though, since each
> frame's state will be stored on a separate FrameNavigationEntry in the browser
> process (and each frame may come from a different renderer process).
> 
> Looking closer, it appears to be used in Browser::ViewSourceForFrame, which
> loads a subframe URL and state at the top-level of a new tab in view-source
mode
> (see browser.cc).  That's potentially a problem, since we'll now have the
> PageState for the entire page rather than the subframe that we're loading in a
> new tab.  (You might even be able to test this out manually, but I'm not sure
> how it will manifest.)
> 
> I wonder if there's a way to get the PageState for just the frame from |data|.

Yep, right-click in iframe - > "View Page Source" shows the parent frame's
content. Maybe we should expose a way to serialize a single WebHistoryItem
without children? I'll play with it.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_entry.cc:92:
entry_->unique_names_to_items_[item.target().utf8()] = this;
On 2014/04/28 16:29:00, Charlie Reis wrote:
> On 2014/04/25 21:49:21, Nate Chapin wrote:
> > When a HistoryNode is created during deserialization, it doesn't have a
> > WebHistoryItem with a valid target, so when updating the item, update the
> > target.
> 
> I'm not sure I follow this, but it also sounds like it would be useful to add
as
> a comment in the code.

Done.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
File content/renderer/history_serialization.cc (right):

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_serialization.cc:83: void
RecursivelyGenerateFrameState(HistoryEntry::HistoryNode* node,
On 2014/04/28 16:29:00, Charlie Reis wrote:
> Just to be sure I'm clear why it's ok for this to return void rather than a
> bool, it looks like the old version could never actually return false, right? 
> (Same below.)

Correct, we never returned false from either of these.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_serialization.cc:176: int main_routing_id) {
On 2014/04/28 16:29:00, Charlie Reis wrote:
> Just want to be sure I understand the reason for passing in the main frame's
> routing ID.  HistoryEntry still assumes it's for the top-level frame and
> includes the entire tree, right?  We'll have to later break it down into
> something frame-specific so that parts of the tree can live in other
processes.

Right. This trick might not be 100% necessary, it's a defense against
mismatching frames when determining how to execute a history navigation.

https://codereview.chromium.org/248013003/diff/40001/content/renderer/history...
content/renderer/history_serialization.cc:179: return 0;
On 2014/04/28 16:29:00, Charlie Reis wrote:
> NULL

Done.

[Nate Chapin, 2014-04-28 23:08:47]

Ok, I think this is ready for another review.

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
content/renderer/history_controller.cc:70: different_document_loads.push_back(
I can't find a compelling reason I used same_document_loads here. This fallback
to a different document load removes the need to manually set the main frame's
routing id here.

[Charlie Reis, 2014-04-29 00:24:36]

Thanks!  Looks like it's ready apart from GoToEntry, which I don't yet
understand.

(Side note: if you need to rebase during reviews, it's easier to review the
difference between patch sets if you upload the rebase in its own patch,
followed by another patch with your changes.  Not a big issue if you forget,
though.)

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
content/renderer/history_controller.cc:70: different_document_loads.push_back(
On 2014/04/28 23:08:47, Nate Chapin wrote:
> I can't find a compelling reason I used same_document_loads here. This
fallback
> to a different document load removes the need to manually set the main frame's
> routing id here.

Hmm, I don't understand this at all.  Can you explain a bit of what this code is
doing?  It's hard for me to tell whether it's correct.  (Now that it's in the
Chrome repository, some comments might help here.)

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
content/renderer/history_entry.cc:96: // The previous HistoryItem might not have
ahad a target set, or it might be
nit: ahad -> had

[Nate Chapin, 2014-04-29 19:31:53]

Sorry about the diffs, I'm bad at remembering to upload after rebase. Feel free
to keep reminding me when I forget :)

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
File content/renderer/history_controller.cc (right):

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
content/renderer/history_controller.cc:70: different_document_loads.push_back(
On 2014/04/29 00:24:37, Charlie Reis wrote:
> On 2014/04/28 23:08:47, Nate Chapin wrote:
> > I can't find a compelling reason I used same_document_loads here. This
> fallback
> > to a different document load removes the need to manually set the main
frame's
> > routing id here.
> 
> Hmm, I don't understand this at all.  Can you explain a bit of what this code
is
> doing?  It's hard for me to tell whether it's correct.  (Now that it's in the
> Chrome repository, some comments might help here.)

Comments added to both modified if() blocks.

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
File content/renderer/history_entry.cc (right):

https://codereview.chromium.org/248013003/diff/80001/content/renderer/history...
content/renderer/history_entry.cc:96: // The previous HistoryItem might not have
ahad a target set, or it might be
On 2014/04/29 00:24:37, Charlie Reis wrote:
> nit: ahad -> had

Done.

[Charlie Reis, 2014-04-29 21:36:55]

Awesome, that helps a ton.  LGTM!

[Nate Chapin, 2014-04-29 21:38:15]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-29 21:41:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/248013003/120001

[commit-bot: I haz the power, 2014-04-29 22:21:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 22:21:18]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[Nate Chapin, 2014-04-29 22:22:33]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-29 22:24:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/248013003/120001

[commit-bot: I haz the power, 2014-04-29 22:38:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 22:38:18]

Try jobs failed on following builders:
  win_chromium_x64_rel on tryserver.chromium

[Nate Chapin, 2014-04-29 22:54:58]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-29 22:56:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/248013003/120001

[commit-bot: I haz the power, 2014-04-29 23:30:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 23:30:25]

Try jobs failed on following builders:
  win_chromium_rel on tryserver.chromium

[Nate Chapin, 2014-04-30 16:16:16]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2014-04-30 16:16:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/248013003/120001

[commit-bot: I haz the power, 2014-04-30 18:26:07]

Change committed as 267274