Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1814993002:
Handle about:blank in the Origin Info Bubble better. (Closed)
|
Created:
4 years, 9 months ago by palmer Modified:
3 years, 10 months ago CC:
chromium-reviews, markusheintz_, msramek+watch_chromium.org, raymes+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHandle about:blank in the Origin Info Bubble better.
If there is an opener, show its cookies, permissions, and connection status.
Otherwise, maintain the status quo (but that will also be improved in a later
CL).
BUG=595520, 469889, 466422
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #
Total comments: 1
Patch Set 2 : A different approach: Get the opener, if any. #Patch Set 3 : Set the right URL and SecurityInfo in the caller, rather than trying to repair in the callee. #Patch Set 4 : Add and use {Get,Set}Origin in NavigationEntry. #
Total comments: 13
Patch Set 5 : No GetOpener at all. #
Total comments: 5
Messages
Total messages: 29 (8 generated)
palmer@chromium.org changed reviewers: + felt@chromium.org, meacer@chromium.org
felt: PTAL meacer: FYI
https://codereview.chromium.org/1814993002/diff/1/chrome/browser/ui/website_s... File chrome/browser/ui/website_settings/website_settings.cc (left): https://codereview.chromium.org/1814993002/diff/1/chrome/browser/ui/website_s... chrome/browser/ui/website_settings/website_settings.cc:375: } This conflicts with https://chromium.googlesource.com/chromium/src/+/60886a6e1591b314f9702fc5adc8..., we explicitly wanted not to show "This is an internal chrome page" for about:blank URLs. Note that about:blank URLs are actually controlled by their opener: w = window.open("about:blank") w.document.write("hello") will modify the contents of the about:blank page.
> we explicitly wanted not to show "This is an internal chrome page" for
> about:blank URLs. Note that about:blank URLs are actually controlled by their
> opener:
>
> w = window.open("about:blank")
> w.document.write("hello")
>
> will modify the contents of the about:blank page.
Ahh. I see.
But that, plus:
if (url.SchemeIs(url::kAboutScheme)) {
// All about: URLs except about:blank are redirected.
DCHECK_EQ(url::kAboutBlankURL, url.spec());
suggests that the bug
(https://bugs.chromium.org/p/chromium/issues/detail?id=595520) is wrong to say
that about: URLs have a problem — it's really limited to data: and blob: ?
And, once an opener writes content or code into the about:blank openee, should
we show the origin of the opener? I note that in an openee, location and
location.origin name the opener, but the OIB shows "about:blank". So, that's
wrong, right?
w = window.open("about:blank")
w.document.write(w.document.cookie)
w.document.write(w.location)
w.document.write(w.location.origin)
On 2016/03/17 22:44:11, palmer wrote:
> > we explicitly wanted not to show "This is an internal chrome page" for
> > about:blank URLs. Note that about:blank URLs are actually controlled by
their
> > opener:
> >
> > w = window.open("about:blank")
> > w.document.write("hello")
> >
> > will modify the contents of the about:blank page.
>
> Ahh. I see.
>
> But that, plus:
>
> if (url.SchemeIs(url::kAboutScheme)) {
> // All about: URLs except about:blank are redirected.
> DCHECK_EQ(url::kAboutBlankURL, url.spec());
>
> suggests that the bug
> (https://bugs.chromium.org/p/chromium/issues/detail?id=595520) is wrong to say
> that about: URLs have a problem — it's really limited to data: and blob: ?
I think about: URLs still have the problem: Changing the permissions don't have
any affect. It would be nice to either make them work or not show the UI.
Ensuring permissions work correctly sounds hard and error prone for about:blank,
but maybe it's still possible? We'll need to differentiate between when the
opener modifies the page vs not if we want to do that.
>
> And, once an opener writes content or code into the about:blank openee, should
> we show the origin of the opener? I note that in an openee, location and
> location.origin name the opener, but the OIB shows "about:blank". So, that's
> wrong, right?
I wanted to do this badly but was asked to go with a simpler approach. There is
a long thread at crbug.com/466422 if you want to chime in.
>
> w = window.open("about:blank")
> w.document.write(w.document.cookie)
> w.document.write(w.location)
> w.document.write(w.location.origin)
Description was changed from ========== In the Origin Info Bubble, treat about: the same as chrome:. Don't show permissions or connection status for about:blank. It's an internal Chrome page. BUG=595520 ========== to ========== Handle about:blank in the Origin Info Bubble better. If there is an opener, show its cookies, permissions, and connection status. Otherwise, maintain the status quo (but that will also be improved in a later CL). BUG=595520,469889 ==========
meacer: See what you think about this approach.
The CQ bit was checked by palmer@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1814993002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1814993002/20001
Lgtm [a screenshot would be nice too :)]
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=595520. But it sounds like there is a more reliable approach I should take.
Description was changed from ========== Handle about:blank in the Origin Info Bubble better. If there is an opener, show its cookies, permissions, and connection status. Otherwise, maintain the status quo (but that will also be improved in a later CL). BUG=595520,469889 ========== to ========== Handle about:blank in the Origin Info Bubble better. If there is an opener, show its cookies, permissions, and connection status. Otherwise, maintain the status quo (but that will also be improved in a later CL). BUG=595520,469889,466422 ==========
On 2016/03/18 20:03:32, palmer (OOO until 2016-03-28) wrote: > Screenshots in https://bugs.chromium.org/p/chromium/issues/detail?id=595520. But > it sounds like there is a more reliable approach I should take. Should also change the title as it's not correct anymore.
> Should also change the title as it's not correct anymore. Done.
palmer@chromium.org changed reviewers: + creis@chromium.org
creis: If I read you correctly in https://bugs.chromium.org/p/chromium/issues/detail?id=595520#c21, you mean that we should add virtual void SetOrigin(const url::OriginL& origin) = 0; virtual const url::Origin& GetOrigin() const = 0; to go with the existing virtual void SetURL(const GURL& url) = 0; virtual const GURL& GetURL() const = 0; in content/public/browser/navigation_entry.{h,cc} and subclasses, right? And that we should update all SetURL call sites to also have calls to SetOrigin? Would that be significantly more robust than the current simple approach: + GURL url = entry->GetURL(); + if (url.SchemeIs(url::kAboutScheme) && contents->HasOpener()) { + // If this is a navigation to about: and has an opener, show the details of + // *that* origin. That is the effective origin, and people need to see that. + contents = contents->GetOpener(); + DCHECK(contents); + entry = contents->GetController().GetVisibleEntry(); + if (entry) + url = entry->GetURL(); + } + ? Would any other calls make use of a GetOrigin?
Sorry for the hassles here, but the opener approach is the wrong direction and we haven't had the APIs for the effective origin before. I think it's possible, but we need to be careful. On 2016/04/06 00:58:54, palmer wrote: > creis: If I read you correctly in > https://bugs.chromium.org/p/chromium/issues/detail?id=595520#c21, you mean that > we should add > > virtual void SetOrigin(const url::OriginL& origin) = 0; > virtual const url::Origin& GetOrigin() const = 0; > > to go with the existing > > virtual void SetURL(const GURL& url) = 0; > virtual const GURL& GetURL() const = 0; > > in content/public/browser/navigation_entry.{h,cc} and subclasses, right? Yes, that's what I was suggesting, but it's not 100% clear if that's the right direction yet. I think we need to have a bit of a design discussion here before proceeding. Maybe it would help for me to write up a doc on the issues involved, to get that discussion going. > And > that we should update all SetURL call sites to also have calls to SetOrigin? No, we would have to be very careful about that. I think it should only be assigned at the time of commit, if we add it. > > Would that be significantly more robust than the current simple approach: > > + GURL url = entry->GetURL(); > + if (url.SchemeIs(url::kAboutScheme) && contents->HasOpener()) { > + // If this is a navigation to about: and has an opener, show the details of > + // *that* origin. That is the effective origin, and people need to see > that. > + contents = contents->GetOpener(); > + DCHECK(contents); > + entry = contents->GetController().GetVisibleEntry(); > + if (entry) > + url = entry->GetURL(); > + } > + Yes, because this approach is broken. WebContents::GetOpener() is not always the effective origin of the about:blank page: 1) The effective origin of the page does not change if the opener closes. 2) The effective origin of the page does not change if the opener navigates cross-site. 3) I think the opener of the main frame can change without changing the effective opener (e.g., if another frame does a window.open("", window_name)). > ? Would any other calls make use of a GetOrigin? There is other code that would want to know the effective origin of the page, but I'm not sure yet if they need it from the NavigationEntry.
On 2016/04/08 19:32:54, Charlie Reis wrote: > Sorry for the hassles here, but the opener approach is the wrong direction and > we haven't had the APIs for the effective origin before. I think it's possible, > but we need to be careful. > > > On 2016/04/06 00:58:54, palmer wrote: > > creis: If I read you correctly in > > https://bugs.chromium.org/p/chromium/issues/detail?id=595520#c21, you mean > that > > we should add > > > > virtual void SetOrigin(const url::OriginL& origin) = 0; > > virtual const url::Origin& GetOrigin() const = 0; > > > > to go with the existing > > > > virtual void SetURL(const GURL& url) = 0; > > virtual const GURL& GetURL() const = 0; > > > > in content/public/browser/navigation_entry.{h,cc} and subclasses, right? > > Yes, that's what I was suggesting, but it's not 100% clear if that's the right > direction yet. I think we need to have a bit of a design discussion here before > proceeding. Maybe it would help for me to write up a doc on the issues > involved, to get that discussion going. > > > > And > > that we should update all SetURL call sites to also have calls to SetOrigin? > > No, we would have to be very careful about that. I think it should only be > assigned at the time of commit, if we add it. > > > > > Would that be significantly more robust than the current simple approach: > > > > + GURL url = entry->GetURL(); > > + if (url.SchemeIs(url::kAboutScheme) && contents->HasOpener()) { > > + // If this is a navigation to about: and has an opener, show the details > of > > + // *that* origin. That is the effective origin, and people need to see > > that. > > + contents = contents->GetOpener(); > > + DCHECK(contents); > > + entry = contents->GetController().GetVisibleEntry(); > > + if (entry) > > + url = entry->GetURL(); > > + } > > + > > Yes, because this approach is broken. WebContents::GetOpener() is not always > the effective origin of the about:blank page: > 1) The effective origin of the page does not change if the opener closes. > 2) The effective origin of the page does not change if the opener navigates > cross-site. > 3) I think the opener of the main frame can change without changing the > effective opener (e.g., if another frame does a window.open("", window_name)). > > > > > ? Would any other calls make use of a GetOrigin? > > There is other code that would want to know the effective origin of the page, > but I'm not sure yet if they need it from the NavigationEntry. Here's the doc I wrote up describing the issues involved: https://docs.google.com/document/d/1xcrOn2v28VrOspSuCTvBTcknvaF8HHAGnJfikUkKZ... Hope that helps!
Description was changed from ========== Handle about:blank in the Origin Info Bubble better. If there is an opener, show its cookies, permissions, and connection status. Otherwise, maintain the status quo (but that will also be improved in a later CL). BUG=595520,469889,466422 ========== to ========== Handle about:blank in the Origin Info Bubble better. If there is an opener, show its cookies, permissions, and connection status. Otherwise, maintain the status quo (but that will also be improved in a later CL). BUG=595520,469889,466422 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Thanks to creis for clues. Not sure if this patchset is entirely right, but it works in manual testing.
creis@chromium.org changed reviewers: + alexmos@chromium.org
[+alexmos for origin conversion question] Better, but some thoughts below. This will also be an important thing to have tests for, both for the bubble and the new GetOrigin call. (The latter can be in navigation_controller_impl_browsertest.cc, and should compare it to GetURL in the about:blank popup case. Maybe also data URLs and a normal URL, just to be safe.) https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:41: if (entry->GetURL().SchemeIs(url::kAboutScheme)) { I'd recommend comparing against kAboutBlankURL (if we keep this check). https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:42: const WebContents* opener = contents->GetOpener(); I tried to point out in the doc that opener is not the right thing to look at to determine origin. There should be no calls to GetOpener() here. I'm guessing this code should look something like: // Show the origin if it's not unique, and otherwise fall back to the URL. Origin origin = entry ? entry->GetOrigin() : ...GetLastCommittedOrigin(); GURL url = GURL(origin.Serialize()); if (origin.unique() && entry) url = entry->GetURL(); https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:51: url = GURL(entry->GetOrigin().Serialize()); Why do this line on both sides of the branch? I could see using entry->GetURL() here, given that we know the URL isn't about:blank. Or we could use GetOrigin() in both cases (outside the conditional), if we trust it to be correct in all cases. https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:55: url = GURL(contents->GetMainFrame()->GetLastCommittedOrigin().Serialize()); Alex, is there a good way to do this, or should this be blocked on moving ShowWebsiteSettings to an Origin rather than a URL? I'm concerned about cases where we try to create a GURL on the string "null". Maybe falling back to the URL for unique origins (as I suggest above) would be sufficient? https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_navigation_entry.h:126: url::Origin origin_; You don't need any changes to this file. https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.h:510: url::Origin origin_; Please put this above browser_initiated_post_data_ and mention that it is not persisted with session restore. Also: // TODO(creis): Move this to FrameNavigationEntry to make it useful for subframes as well.
Thanks for the clues. If you think this upcoming patchset makes any sense at all, I'll start writing tests and looking into the TODO(palmer). https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:41: if (entry->GetURL().SchemeIs(url::kAboutScheme)) { On 2016/04/18 19:58:38, Charlie Reis wrote: > I'd recommend comparing against kAboutBlankURL (if we keep this check). Acknowledged. https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:42: const WebContents* opener = contents->GetOpener(); > I tried to point out in the doc that opener is not the right thing to look at to > determine origin. There should be no calls to GetOpener() here. I'm sorry; I misunderstood you as saying there might not be an opener and we have to do something else if not. Anyway: > I'm guessing this code should look something like: > > // Show the origin if it's not unique, and otherwise fall back to the URL. > Origin origin = entry ? entry->GetOrigin() : ...GetLastCommittedOrigin(); > GURL url = GURL(origin.Serialize()); > if (origin.unique() && entry) > url = entry->GetURL(); This resulted in showing "about:blank" for a tab that results from calling window.open("about:blank") from a google.com page. (It showed "google.com" for a tab that resulted from calling window.open(""), however.) I currently have: url::Origin origin = entry ? entry->GetOrigin() : contents->GetMainFrame()->GetLastCommittedOrigin(); GURL url(origin.Serialize()); if (entry && origin.unique()) { url = GURL(contents->GetMainFrame()->GetLastCommittedOrigin().Serialize()); } which works correctly (showing "google.com" for both window.open("about:blank") and window.open(""), including after closing the google.com opener) in manual testing. (It shows "" for when you open a fresh tab and navigate to about:blank in the Omnibox. I have a TODO about that.) https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:51: url = GURL(entry->GetOrigin().Serialize()); On 2016/04/18 19:58:38, Charlie Reis wrote: > Why do this line on both sides of the branch? > > I could see using entry->GetURL() here, given that we know the URL isn't > about:blank. Or we could use GetOrigin() in both cases (outside the > conditional), if we trust it to be correct in all cases. Acknowledged. https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... content/browser/frame_host/frame_navigation_entry.h:126: url::Origin origin_; On 2016/04/18 19:58:38, Charlie Reis wrote: > You don't need any changes to this file. Done. https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/1814993002/diff/60001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.h:510: url::Origin origin_; On 2016/04/18 19:58:38, Charlie Reis wrote: > Please put this above browser_initiated_post_data_ and mention that it is not > persisted with session restore. Also: > > // TODO(creis): Move this to FrameNavigationEntry to make it useful for > subframes as well. Done.
https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:55: url = GURL(contents->GetMainFrame()->GetLastCommittedOrigin().Serialize()); On 2016/04/18 19:58:38, Charlie Reis wrote: > Alex, is there a good way to do this, or should this be blocked on moving > ShowWebsiteSettings to an Origin rather than a URL? I'm concerned about cases > where we try to create a GURL on the string "null". > > Maybe falling back to the URL for unique origins (as I suggest above) would be > sufficient? Yeah, unfortunately this is the best you can do right now, and ideally we'd move ShowWebsiteSettings to use origins instead. If we wanted something in the meantime, having a similar special case to fall back to the URL for unique origins seems ok. (Though which URL would you use here?) The situation we want to avoid in general is constructing two GURLs like this out of unique origins and then later on comparing them and concluding equivalency (since both GURLs will fail validation and normalize to the empty URL). This is probably less of an issue here, since the url seems to be for display purposes, though I haven't looked into how ShowWebsiteSettings works. Random question -- would all of this work properly for file:/// URLs?
> > Maybe falling back to the URL for unique origins (as I suggest above) would be > > sufficient? > > Yeah, unfortunately this is the best you can do right now, and ideally we'd move > ShowWebsiteSettings to use origins instead. If we wanted something in the I'm kind of thinking I should do that in this CL. > Random question -- would all of this work properly for file:/// URLs? Ahh, good question. This CL creates a bug: Currently, file:///etc appears as "file:///etc", but with this patch we get "file:///". So I'll have to fix that.
Thanks! A few more thoughts below. On 2016/04/19 00:28:08, palmer wrote: > > > Maybe falling back to the URL for unique origins (as I suggest above) would > be > > > sufficient? > > > > Yeah, unfortunately this is the best you can do right now, and ideally we'd > move > > ShowWebsiteSettings to use origins instead. If we wanted something in the > > I'm kind of thinking I should do that in this CL. We may need to be careful if we do-- I bet that would break data URLs and other URLs with unique origins, where we'll show "" instead of the actual URL. I suppose we could pass both the origin and URL and let it decide, rather than doing the hard work here. Seems like a judgement call. > > Random question -- would all of this work properly for file:/// URLs? > > Ahh, good question. This CL creates a bug: Currently, file:///etc appears as > "file:///etc", but with this patch we get "file:///". So I'll have to fix that. Interesting. So unique origins aren't the only ones where we might want to fall back to the URL. Seems tricky. https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/location_bar/location_icon_view.cc (right): https://codereview.chromium.org/1814993002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/location_bar/location_icon_view.cc:42: const WebContents* opener = contents->GetOpener(); On 2016/04/18 23:37:35, palmer wrote: > > I tried to point out in the doc that opener is not the right thing to look at > to > > determine origin. There should be no calls to GetOpener() here. > > I'm sorry; I misunderstood you as saying there might not be an opener and we > have to do something else if not. Anyway: > > > I'm guessing this code should look something like: > > > > // Show the origin if it's not unique, and otherwise fall back to the URL. > > Origin origin = entry ? entry->GetOrigin() : ...GetLastCommittedOrigin(); > > GURL url = GURL(origin.Serialize()); > > if (origin.unique() && entry) > > url = entry->GetURL(); > > This resulted in showing "about:blank" for a tab that results from calling > window.open("about:blank") from a http://google.com page. (It showed "google.com" for a > tab that resulted from calling window.open(""), however.) > > I currently have: > > url::Origin origin = entry > ? entry->GetOrigin() > : contents->GetMainFrame()->GetLastCommittedOrigin(); > GURL url(origin.Serialize()); > if (entry && origin.unique()) { > url = GURL(contents->GetMainFrame()->GetLastCommittedOrigin().Serialize()); > } > > which works correctly (showing "google.com" for both window.open("about:blank") > and window.open(""), including after closing the http://google.com opener) in manual > testing. This is probably due to the bug I mentioned in your NavigationControllerImpl change, since we're setting the last committed entry's origin to the URL, not to the effective origin. Can you switch back to the version I suggested if that's fixed? > (It shows "" for when you open a fresh tab and navigate to about:blank > in the Omnibox. I have a TODO about that.) Yep, we'll want to show about:blank there. Maybe something like: if (origin.unique() { if (entry) url = entry->GetURL(); else url = GURL(kAboutBlankURL); } https://codereview.chromium.org/1814993002/diff/80001/content/browser/frame_h... File content/browser/frame_host/frame_navigation_entry.h (right): https://codereview.chromium.org/1814993002/diff/80001/content/browser/frame_h... content/browser/frame_host/frame_navigation_entry.h:15: #include "url/origin.h" nit: No changes needed to this file. https://codereview.chromium.org/1814993002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/1814993002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_controller_impl.cc:1116: new_entry->SetOrigin(url::Origin(params.url)); Oh, I missed this before. This is wrong-- it'll be about:blank in the case we care about. Instead, this should be params.origin, so that we capture the effective origin. (I think it's worth adding the tests now, since they would have caught this.) Same in the cases below. https://codereview.chromium.org/1814993002/diff/80001/content/browser/frame_h... File content/browser/frame_host/navigation_entry_impl.h (right): https://codereview.chromium.org/1814993002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.h:408: // The effective Origin at the time of commit. Let's add that it starts as an empty, unique origin (until the entry commits). https://codereview.chromium.org/1814993002/diff/80001/content/browser/frame_h... content/browser/frame_host/navigation_entry_impl.h:410: // This member is not persisted with session restore because it is transient. nit: Drop "because it is transient." Also, no new paragraph needed. https://codereview.chromium.org/1814993002/diff/80001/content/public/browser/... File content/public/browser/navigation_entry.h (right): https://codereview.chromium.org/1814993002/diff/80001/content/public/browser/... content/public/browser/navigation_entry.h:61: // The effective Origin at the time of commit. Same about mentioning that it's an empty unique origin before the entry commits.
palmer: In case you are looking for more CLs to close, this is a good candidate. The origin info was removed from the OIB so this CL won't work anymore. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
