Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3)

Issue 1407853005: OOPIF: Add frame_unique_name to FrameNavigationEntry. (Closed)

Created:
5 years, 1 month ago by Charlie Reis
Modified:
5 years, 1 month ago
CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

OOPIF: Add frame_unique_name to FrameNavigationEntry. This holds the unique name of the frame in which the navigation took place. This is stable after back/forward and restore, whereas the FrameTreeNode ID will change over time. BUG=236848, 502317 TEST=No behavior change yet. Committed: https://crrev.com/6d752c35ee308bc817d80a7de94a77e6cdefaa48 Cr-Commit-Position: refs/heads/master@{#358746}

Patch Set 1 #

Patch Set 2 : Add test #

Patch Set 3 : Update comments #

Total comments: 13

Patch Set 4 : Rename target to frame_unique_name #

Total comments: 2

Patch Set 5 : Add uniqueness comment #

Total comments: 2

Patch Set 6 : const ref #

Unified diffs Side-by-side diffs Delta from patch set Stats (+149 lines, -32 lines) Patch
M content/browser/frame_host/frame_navigation_entry.h View 1 2 3 4 5 4 chunks +24 lines, -9 lines 0 comments Download
M content/browser/frame_host/frame_navigation_entry.cc View 1 2 3 4 5 2 chunks +8 lines, -3 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl.cc View 1 2 3 4 chunks +6 lines, -5 lines 0 comments Download
M content/browser/frame_host/navigation_controller_impl_browsertest.cc View 1 2 3 2 chunks +92 lines, -4 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M content/browser/frame_host/navigation_entry_impl.cc View 1 2 3 4 5 4 chunks +13 lines, -11 lines 0 comments Download
M content/common/frame_messages.h View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/frame_navigate_params.h View 1 2 3 1 chunk +3 lines, -0 lines 0 comments Download
M content/renderer/render_frame_impl.cc View 1 2 3 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 26 (8 generated)
Charlie Reis
Avi, can you take a look? The target holds the unique name of the frame, ...
5 years, 1 month ago (2015-11-09 18:17:05 UTC) #2
Avi (use Gerrit)
I don't have an issue with the code, but the naming... https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): ...
5 years, 1 month ago (2015-11-09 20:58:26 UTC) #3
Charlie Reis
https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h#newcode61 content/browser/frame_host/frame_navigation_entry.h:61: // Only the main frame can have an empty ...
5 years, 1 month ago (2015-11-09 21:28:48 UTC) #4
Avi (use Gerrit)
https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h#newcode61 content/browser/frame_host/frame_navigation_entry.h:61: // Only the main frame can have an empty ...
5 years, 1 month ago (2015-11-09 21:30:58 UTC) #5
Charlie Reis
https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h#newcode61 content/browser/frame_host/frame_navigation_entry.h:61: // Only the main frame can have an empty ...
5 years, 1 month ago (2015-11-09 21:38:41 UTC) #6
Avi (use Gerrit)
https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h#newcode61 content/browser/frame_host/frame_navigation_entry.h:61: // Only the main frame can have an empty ...
5 years, 1 month ago (2015-11-09 21:44:40 UTC) #7
Charlie Reis
Nate, can I get your thoughts on the naming question below? https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): ...
5 years, 1 month ago (2015-11-09 21:49:24 UTC) #9
Nate Chapin
https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h#newcode61 content/browser/frame_host/frame_navigation_entry.h:61: // Only the main frame can have an empty ...
5 years, 1 month ago (2015-11-09 21:52:23 UTC) #10
Charlie Reis
Avi, PTAL. Daniel, can you look at frame_messages.h for IPC owners? https://codereview.chromium.org/1407853005/diff/40001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): ...
5 years, 1 month ago (2015-11-09 22:19:41 UTC) #13
Avi (use Gerrit)
LGTM; comment clarification about the uniqueness? https://codereview.chromium.org/1407853005/diff/60001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/60001/content/browser/frame_host/frame_navigation_entry.h#newcode62 content/browser/frame_host/frame_navigation_entry.h:62: // Only the ...
5 years, 1 month ago (2015-11-09 22:23:17 UTC) #14
Charlie Reis
Oops, thanks for the reminder. https://codereview.chromium.org/1407853005/diff/60001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/60001/content/browser/frame_host/frame_navigation_entry.h#newcode62 content/browser/frame_host/frame_navigation_entry.h:62: // Only the main ...
5 years, 1 month ago (2015-11-09 22:29:21 UTC) #15
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407853005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407853005/80001
5 years, 1 month ago (2015-11-09 22:30:12 UTC) #17
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 1 month ago (2015-11-09 23:51:37 UTC) #19
dcheng
lgtm with nits https://codereview.chromium.org/1407853005/diff/80001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/80001/content/browser/frame_host/frame_navigation_entry.h#newcode43 content/browser/frame_host/frame_navigation_entry.h:43: void UpdateEntry(std::string frame_unique_name, Nit: const std::string& ...
5 years, 1 month ago (2015-11-09 23:53:33 UTC) #20
Charlie Reis
Thanks! https://codereview.chromium.org/1407853005/diff/80001/content/browser/frame_host/frame_navigation_entry.h File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1407853005/diff/80001/content/browser/frame_host/frame_navigation_entry.h#newcode43 content/browser/frame_host/frame_navigation_entry.h:43: void UpdateEntry(std::string frame_unique_name, On 2015/11/09 23:53:33, dcheng wrote: ...
5 years, 1 month ago (2015-11-10 00:08:01 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407853005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407853005/100001
5 years, 1 month ago (2015-11-10 00:10:32 UTC) #24
commit-bot: I haz the power
Committed patchset #6 (id:100001)
5 years, 1 month ago (2015-11-10 01:59:55 UTC) #25
commit-bot: I haz the power
5 years, 1 month ago (2015-11-10 02:01:10 UTC) #26
Message was sent while issue was closed.
Patchset 6 (id:??) landed as
https://crrev.com/6d752c35ee308bc817d80a7de94a77e6cdefaa48
Cr-Commit-Position: refs/heads/master@{#358746}

Powered by Google App Engine
This is Rietveld 408576698