Reloading page after installing app should bring it into correct process.

Reloading page after installing app should bring it into correct process.

BUG=80621
TEST=Open a page which is part of an app, in another tab install the app, reload the original page. The renderer process for the tab should be switched.


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

[nasko, 2012-01-26 00:37:22]

This change fixes issue 80621, but covers only browser initiated reloads. 

JavaScript reloads need support to be added to the renderer process so it is
aware of its privilege level.

[Charlie Reis, 2012-01-26 01:20:22]

Thanks!

https://chromiumcodereview.appspot.com/9169065/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/app_process_apitest.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/1/chrome/browser/extensio...
chrome/browser/extensions/app_process_apitest.cc:451: *
We don't use multi-line /* */ comments much.

You could switch this to //, but I think it might be nicer to separate out the
JS reload into a separate test that remains disabled.  You'll just have to
duplicate the setup from lines 392 to 414.

Also, I noticed that we did have a check in the original fix for 80621 that
might work, so we might be able to fix it all in this CL:
http://codereview.chromium.org/7063015/diff/3001/chrome/renderer/chrome_conte...
That might not be a perfect fix, though (e.g., might not catch upgrading from a
hosted app to an isolated app)...

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:268:
NavigationEntryImpl* entry = entries_[current_index].get();
GetEntryAtIndex

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:273: // the reload
must happen in a new process and behave as new navigation
Nit: let's split out the "behave as new navigation" into a separate sentence
that says a bit more.  It's important to note that we need a new entry with a
different SiteInstance and fresh page_id, so we treat it as a new navigation
(which happens to clear the forward history).

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:275: if
(site_instance != NULL &&
More common to drop the != NULL.  And actually, should we be asserting that it
isn't NULL?  If we're reloading an entry, I would never expect it to have a NULL
SiteInstance.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:277: // Create a
navigation entry that resembles the current one
Nit: perhaps mention which things must not be copied (page ID, SiteInstance, and
content_state), and end comments with a period.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:609:
details->did_replace_entry =  pending_entry_->is_cross_site_reload();
Nit: extra space.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
File content/browser/tab_contents/navigation_entry_impl.h (right):

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_entry_impl.h:146: void
set_is_cross_site_reload(bool is_cross_site_reload) {
Let's add a comment here.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_entry_impl.h:202: // site instances, it
has to be reloaded in a different site instance. In
This sentence is a bit awkward.  Maybe the first "site instances" should be
something else, like "the state of the URL"?  Also, we should mention that this
value isn't persisted or needed after the entry commits.

[nasko, 2012-01-26 20:25:57]

I've added the check for mismatched process types in the case of Javascript
reload, which detects web page to extension and reverse transition. Therefore I
also enabled the javascript browser test.

https://chromiumcodereview.appspot.com/9169065/diff/1/chrome/browser/extensio...
File chrome/browser/extensions/app_process_apitest.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/1/chrome/browser/extensio...
chrome/browser/extensions/app_process_apitest.cc:451: *
On 2012/01/26 01:20:23, creis wrote:
> We don't use multi-line /* */ comments much.
> 
> You could switch this to //, but I think it might be nicer to separate out the
> JS reload into a separate test that remains disabled.  You'll just have to
> duplicate the setup from lines 392 to 414.
> 
> Also, I noticed that we did have a check in the original fix for 80621 that
> might work, so we might be able to fix it all in this CL:
>
http://codereview.chromium.org/7063015/diff/3001/chrome/renderer/chrome_conte...
> That might not be a perfect fix, though (e.g., might not catch upgrading from
a
> hosted app to an isolated app)...

I've added the check and works to detect regular web page to hosted app
transition and reverse, which makes this code happy. I've uncommented it.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:268:
NavigationEntryImpl* entry = entries_[current_index].get();
On 2012/01/26 01:20:23, creis wrote:
> GetEntryAtIndex

I've used direct access to the collection, since I get back a
NavigationEntryImpl*, whereas this call returns NavigationEntry*. Since site
instance is not publicly available through NavigationEntry*, I would need to
cast it. This version seems more intuitive to get the proper pointer type back.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:273: // the reload
must happen in a new process and behave as new navigation
On 2012/01/26 01:20:23, creis wrote:
> Nit: let's split out the "behave as new navigation" into a separate sentence
> that says a bit more.  It's important to note that we need a new entry with a
> different SiteInstance and fresh page_id, so we treat it as a new navigation
> (which happens to clear the forward history).

Done.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:275: if
(site_instance != NULL &&
On 2012/01/26 01:20:23, creis wrote:
> More common to drop the != NULL.  And actually, should we be asserting that it
> isn't NULL?  If we're reloading an entry, I would never expect it to have a
NULL
> SiteInstance.

Done.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:277: // Create a
navigation entry that resembles the current one
On 2012/01/26 01:20:23, creis wrote:
> Nit: perhaps mention which things must not be copied (page ID, SiteInstance,
and
> content_state), and end comments with a period.

Done.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
File content/browser/tab_contents/navigation_entry_impl.h (right):

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_entry_impl.h:146: void
set_is_cross_site_reload(bool is_cross_site_reload) {
On 2012/01/26 01:20:23, creis wrote:
> Let's add a comment here.

Done.

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_entry_impl.h:202: // site instances, it
has to be reloaded in a different site instance. In
On 2012/01/26 01:20:23, creis wrote:
> This sentence is a bit awkward.  Maybe the first "site instances" should be
> something else, like "the state of the URL"?  Also, we should mention that
this
> value isn't persisted or needed after the entry commits.

Done.

[Charlie Reis, 2012-01-26 21:49:45]

LGTM with the changes below.

John, can you review for owner's approval?

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/1/content/browser/tab_con...
content/browser/tab_contents/navigation_controller_impl.cc:268:
NavigationEntryImpl* entry = entries_[current_index].get();
On 2012/01/26 20:25:57, nasko wrote:
> On 2012/01/26 01:20:23, creis wrote:
> > GetEntryAtIndex
> 
> I've used direct access to the collection, since I get back a
> NavigationEntryImpl*, whereas this call returns NavigationEntry*. Since site
> instance is not publicly available through NavigationEntry*, I would need to
> cast it. This version seems more intuitive to get the proper pointer type
back.

Ah.  Yes, that's fine then.  I've emailed John about adding SiteInstance to the
public NavigationEntry interface, but that will be for a separate CL.

https://chromiumcodereview.appspot.com/9169065/diff/5001/content/browser/tab_...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/5001/content/browser/tab_...
content/browser/tab_contents/navigation_controller_impl.cc:272: // If we are
reloading an entry that no longer belongs to the current
Nit: belongs to -> belongs in

https://chromiumcodereview.appspot.com/9169065/diff/5001/content/browser/tab_...
content/browser/tab_contents/navigation_controller_impl.cc:610: if
(pending_entry_ != NULL) {
Nit: no braces on a one line if.

[jam, 2012-01-27 18:15:19]

can brett look at this instead of me? i don't know this code at all.

[Charlie Reis, 2012-01-27 18:22:22]

On 2012/01/27 18:15:19, John Abd-El-Malek wrote:
> can brett look at this instead of me? i don't know this code at all.

Brett's on leave, right?  I've CC'd just in case.  Darin, could you review for
owner's approval?

[(Do not use) nasko, 2012-01-27 18:34:35]

There will be a slight change, one pointer type SiteInstance will move to
SiteInstanceImpl, because of refactoring of public API for site instances.

On Fri, Jan 27, 2012 at 10:22 AM, <creis@chromium.org> wrote:

> On 2012/01/27 18:15:19, John Abd-El-Malek wrote:
>
>> can brett look at this instead of me? i don't know this code at all.
>>
>
> Brett's on leave, right?  I've CC'd just in case.  Darin, could you review
> for
>
> owner's approval?
>
>
https://chromiumcodereview.**appspot.com/9169065/<https://chromiumcodereview....
>

[darin (slow to review), 2012-01-27 19:34:11]

Add a unit test in navigation_controller_impl_unittest.cc?

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:763: if (old_url == new_url) {
This condition isn't necessarily limited to a reload.  You can also get this by
hitting enter in the location bar.

(Generally, reload is reserved to correspond to clicking the "reload" button.)

Also, are you sure comparing URLs is enough?  Maybe we should be comparing site
instances?  What about if the URLs are the same but only differ in terms of
their reference fragment?

Also, isn't this abusing the meaning of CrossesExtensionExtents?  It seems like
if the before and after URL are identical, then it cannot possibly cross
extension extents.  Is this function misnamed?  Given the current name, I
wouldn't be surprised if the caller performed an URL equivalency test as a
short-cut around having to call this function.

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
content/browser/tab_contents/navigation_controller_impl.cc:280: // copy page id,
site instance, and content state.
normally when you reload a page, webkit restores the scroll position of the
page.  however, scroll position is a property of content state.  with this
change, won't we lose scroll position?  is this desired?

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
content/browser/tab_contents/navigation_controller_impl.cc:287: pending_entry_ =
nav_entry;
you don't have to set the PAGE_TRANSITION_RELOAD transition type here?

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
content/browser/tab_contents/navigation_controller_impl.cc:610: // effect of
removing forward browsing history, if such existed.
normally hitting reload does not wipe out forward history.  why do we want to do
that?

[nasko, 2012-01-27 20:14:55]

Responding to the comments from Darin.

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:763: if (old_url == new_url) {
On 2012/01/27 19:34:11, darin wrote:
> This condition isn't necessarily limited to a reload.  You can also get this
by
> hitting enter in the location bar.
> 
> (Generally, reload is reserved to correspond to clicking the "reload" button.)
> 
> Also, are you sure comparing URLs is enough?  Maybe we should be comparing
site
> instances?  What about if the URLs are the same but only differ in terms of
> their reference fragment?
> 
> Also, isn't this abusing the meaning of CrossesExtensionExtents?  It seems
like
> if the before and after URL are identical, then it cannot possibly cross
> extension extents.  Is this function misnamed?  Given the current name, I
> wouldn't be surprised if the caller performed an URL equivalency test as a
> short-cut around having to call this function.

Hitting enter in the location bar is handled by the browser process, as well as
clicking on the reload button. This class is running in the renderer process, so
it doesn't come into picture for those two actions.

Site instance data is also not available in the renderer process, so we can't
really compare them. This code is added to handle renderer initiated reloads,
one instance of which is dong location.reload() in JS. I've discussed with
Charlie that we need better way to express privilege level for renderer
processes, so they can make decision whether to route top level navigation
events to the browser or not. Until we do this work, we can handle JS reload
here. So yes, I agree this can be much better, but it is a much bigger effort,
belonging to a separate issue.

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
content/browser/tab_contents/navigation_controller_impl.cc:287: pending_entry_ =
nav_entry;
On 2012/01/27 19:34:11, darin wrote:
> you don't have to set the PAGE_TRANSITION_RELOAD transition type here?

Again, due to the fact that we are transitioning processes, this is not really a
reload.

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
content/browser/tab_contents/navigation_controller_impl.cc:610: // effect of
removing forward browsing history, if such existed.
On 2012/01/27 19:34:11, darin wrote:
> normally hitting reload does not wipe out forward history.  why do we want to
do
> that?

Navigation entries are identified uniquely by a tuple of page id and site
instance. Since we are changing the site instance, we have potential for page id
conflicts, especially if we have forward history. Charlie and I went through all
the cases of navigating back and forth with reloads and agreed that this is the
safest approach, albeit with a bit of sacrifice in consistent behavior.

[Charlie Reis, 2012-01-27 20:50:18]

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
File content/browser/tab_contents/navigation_controller_impl.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
content/browser/tab_contents/navigation_controller_impl.cc:280: // copy page id,
site instance, and content state.
On 2012/01/27 19:34:11, darin wrote:
> normally when you reload a page, webkit restores the scroll position of the
> page.  however, scroll position is a property of content state.  with this
> change, won't we lose scroll position?  is this desired?

It's not ideal, but I think it's reasonable in the case that a normal web page
is being reloaded after being installed as an app.  The new version of the page
may have new capabilities and should probably appear as a fresh load rather than
a normal reload.

https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab...
content/browser/tab_contents/navigation_controller_impl.cc:610: // effect of
removing forward browsing history, if such existed.
On 2012/01/27 20:14:56, nasko wrote:
> On 2012/01/27 19:34:11, darin wrote:
> > normally hitting reload does not wipe out forward history.  why do we want
to
> do
> > that?
> 
> Navigation entries are identified uniquely by a tuple of page id and site
> instance. Since we are changing the site instance, we have potential for page
id
> conflicts, especially if we have forward history. Charlie and I went through
all
> the cases of navigating back and forth with reloads and agreed that this is
the
> safest approach, albeit with a bit of sacrifice in consistent behavior.

That's right.  We actually had a previous fix for this that preserved the
forward history, but it led to deeper conflicts with page IDs that caused
renderer crashes.  There's more history on http://crbug.com/80621 if you're
interested.

[darin (slow to review), 2012-01-27 23:54:06]

OK, it seems a shame to have artifacts like these leak through.
 Multi-process is supposed to be transparent.  The page was just reloaded
afterall.

Nasko, you can simulate hitting enter in the location bar, by simply doing
"location = location" in script.  I don't believe that  is equivalent to
history.reload().  I believe the latter affects cache policy, forcing
end-to-end validation, whereas the former does not.  (I may be wrong!)

-Darin

On Fri, Jan 27, 2012 at 12:50 PM, <creis@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9169065/diff/**
>
11001/content/browser/tab_**contents/navigation_**controller_impl.cc<https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab_contents/navigation_controller_impl.cc>
> File content/browser/tab_contents/**navigation_controller_impl.cc (right):
>
> https://chromiumcodereview.**appspot.com/9169065/diff/**
> 11001/content/browser/tab_**contents/navigation_**
>
controller_impl.cc#newcode280<https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab_contents/navigation_controller_impl.cc#newcode280>
> content/browser/tab_contents/**navigation_controller_impl.cc:**280: //
> copy
> page id, site instance, and content state.
> On 2012/01/27 19:34:11, darin wrote:
>
>> normally when you reload a page, webkit restores the scroll position
>>
> of the
>
>> page.  however, scroll position is a property of content state.  with
>>
> this
>
>> change, won't we lose scroll position?  is this desired?
>>
>
> It's not ideal, but I think it's reasonable in the case that a normal
> web page is being reloaded after being installed as an app.  The new
> version of the page may have new capabilities and should probably appear
> as a fresh load rather than a normal reload.
>
>
> https://chromiumcodereview.**appspot.com/9169065/diff/**
> 11001/content/browser/tab_**contents/navigation_**
>
controller_impl.cc#newcode610<https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab_contents/navigation_controller_impl.cc#newcode610>
> content/browser/tab_contents/**navigation_controller_impl.cc:**610: //
> effect of removing forward browsing history, if such existed.
> On 2012/01/27 20:14:56, nasko wrote:
>
>> On 2012/01/27 19:34:11, darin wrote:
>> > normally hitting reload does not wipe out forward history.  why do
>>
> we want to
>
>> do
>> > that?
>>
>
>  Navigation entries are identified uniquely by a tuple of page id and
>>
> site
>
>> instance. Since we are changing the site instance, we have potential
>>
> for page id
>
>> conflicts, especially if we have forward history. Charlie and I went
>>
> through all
>
>> the cases of navigating back and forth with reloads and agreed that
>>
> this is the
>
>> safest approach, albeit with a bit of sacrifice in consistent
>>
> behavior.
>
> That's right.  We actually had a previous fix for this that preserved
> the forward history, but it led to deeper conflicts with page IDs that
> caused renderer crashes.  There's more history on http://crbug.com/80621
> if you're interested.
>
>
https://chromiumcodereview.**appspot.com/9169065/<https://chromiumcodereview....
>

[darin (slow to review), 2012-01-27 23:58:35]

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:763: if (old_url == new_url) {
On 2012/01/27 20:14:56, nasko wrote:
> On 2012/01/27 19:34:11, darin wrote:
> > This condition isn't necessarily limited to a reload.  You can also get this
> by
> > hitting enter in the location bar.
> > 
> > (Generally, reload is reserved to correspond to clicking the "reload"
button.)
> > 
> > Also, are you sure comparing URLs is enough?  Maybe we should be comparing
> site
> > instances?  What about if the URLs are the same but only differ in terms of
> > their reference fragment?
> > 
> > Also, isn't this abusing the meaning of CrossesExtensionExtents?  It seems
> like
> > if the before and after URL are identical, then it cannot possibly cross
> > extension extents.  Is this function misnamed?  Given the current name, I
> > wouldn't be surprised if the caller performed an URL equivalency test as a
> > short-cut around having to call this function.

^^^ what about this last point?  is this really the right location for this?


> Hitting enter in the location bar is handled by the browser process, as well
as
> clicking on the reload button. This class is running in the renderer process,
so
> it doesn't come into picture for those two actions.
> 
> Site instance data is also not available in the renderer process, so we can't
> really compare them. This code is added to handle renderer initiated reloads,
> one instance of which is dong location.reload() in JS. I've discussed with
> Charlie that we need better way to express privilege level for renderer
> processes, so they can make decision whether to route top level navigation
> events to the browser or not. Until we do this work, we can handle JS reload
> here. So yes, I agree this can be much better, but it is a much bigger effort,
> belonging to a separate issue.

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:766: return true;
what about the reverse case?  what if the app is uninstalled and the reloaded?

[nasko, 2012-01-28 01:43:12]

Responses to Darin's comments.

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:763: if (old_url == new_url) {
On 2012/01/27 23:58:35, darin wrote:
> On 2012/01/27 20:14:56, nasko wrote:
> > On 2012/01/27 19:34:11, darin wrote:
> > > This condition isn't necessarily limited to a reload.  You can also get
this
> > by
> > > hitting enter in the location bar.
> > > 
> > > (Generally, reload is reserved to correspond to clicking the "reload"
> button.)
> > > 
> > > Also, are you sure comparing URLs is enough?  Maybe we should be comparing
> > site
> > > instances?  What about if the URLs are the same but only differ in terms
of
> > > their reference fragment?
> > > 
> > > Also, isn't this abusing the meaning of CrossesExtensionExtents?  It seems
> > like
> > > if the before and after URL are identical, then it cannot possibly cross
> > > extension extents.  Is this function misnamed?  Given the current name, I
> > > wouldn't be surprised if the caller performed an URL equivalency test as a
> > > short-cut around having to call this function.
> 
> ^^^ what about this last point?  is this really the right location for this?
> 
> 
> > Hitting enter in the location bar is handled by the browser process, as well
> as
> > clicking on the reload button. This class is running in the renderer
process,
> so
> > it doesn't come into picture for those two actions.
> > 
> > Site instance data is also not available in the renderer process, so we
can't
> > really compare them. This code is added to handle renderer initiated
reloads,
> > one instance of which is dong location.reload() in JS. I've discussed with
> > Charlie that we need better way to express privilege level for renderer
> > processes, so they can make decision whether to route top level navigation
> > events to the browser or not. Until we do this work, we can handle JS reload
> > here. So yes, I agree this can be much better, but it is a much bigger
effort,
> > belonging to a separate issue.
> 

It could be moved to the function that calls this, which is named more
appropriately (ShouldFork), though we are indeed crossing a boundary, which
involves time. If you believe it is better in the caller, I'd be happy to move
it. I'm still trying to learn the componentization and how things fit together :
).

https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:766: return true;
On 2012/01/27 23:58:35, darin wrote:
> what about the reverse case?  what if the app is uninstalled and the reloaded?

Yes, if the process "type" mismatches with the current expectation, it will
detect it and route to the browser.

[(Do not use) nasko, 2012-01-28 01:45:54]

Thanks for the info! I will try the "location = location" and test it out
on Monday.

On Fri, Jan 27, 2012 at 3:54 PM, Darin Fisher <darin@chromium.org> wrote:

> OK, it seems a shame to have artifacts like these leak through.
>  Multi-process is supposed to be transparent.  The page was just reloaded
> afterall.
>
> Nasko, you can simulate hitting enter in the location bar, by simply doing
> "location = location" in script.  I don't believe that  is equivalent to
> history.reload().  I believe the latter affects cache policy, forcing
> end-to-end validation, whereas the former does not.  (I may be wrong!)
>
> -Darin
>
>
> On Fri, Jan 27, 2012 at 12:50 PM, <creis@chromium.org> wrote:
>
>>
>> https://chromiumcodereview.**appspot.com/9169065/diff/**
>>
11001/content/browser/tab_**contents/navigation_**controller_impl.cc<https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab_contents/navigation_controller_impl.cc>
>> File content/browser/tab_contents/**navigation_controller_impl.cc
>> (right):
>>
>> https://chromiumcodereview.**appspot.com/9169065/diff/**
>> 11001/content/browser/tab_**contents/navigation_**
>>
controller_impl.cc#newcode280<https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab_contents/navigation_controller_impl.cc#newcode280>
>> content/browser/tab_contents/**navigation_controller_impl.cc:**280: //
>> copy
>> page id, site instance, and content state.
>> On 2012/01/27 19:34:11, darin wrote:
>>
>>> normally when you reload a page, webkit restores the scroll position
>>>
>> of the
>>
>>> page.  however, scroll position is a property of content state.  with
>>>
>> this
>>
>>> change, won't we lose scroll position?  is this desired?
>>>
>>
>> It's not ideal, but I think it's reasonable in the case that a normal
>> web page is being reloaded after being installed as an app.  The new
>> version of the page may have new capabilities and should probably appear
>> as a fresh load rather than a normal reload.
>>
>>
>> https://chromiumcodereview.**appspot.com/9169065/diff/**
>> 11001/content/browser/tab_**contents/navigation_**
>>
controller_impl.cc#newcode610<https://chromiumcodereview.appspot.com/9169065/diff/11001/content/browser/tab_contents/navigation_controller_impl.cc#newcode610>
>> content/browser/tab_contents/**navigation_controller_impl.cc:**610: //
>> effect of removing forward browsing history, if such existed.
>> On 2012/01/27 20:14:56, nasko wrote:
>>
>>> On 2012/01/27 19:34:11, darin wrote:
>>> > normally hitting reload does not wipe out forward history.  why do
>>>
>> we want to
>>
>>> do
>>> > that?
>>>
>>
>>  Navigation entries are identified uniquely by a tuple of page id and
>>>
>> site
>>
>>> instance. Since we are changing the site instance, we have potential
>>>
>> for page id
>>
>>> conflicts, especially if we have forward history. Charlie and I went
>>>
>> through all
>>
>>> the cases of navigating back and forth with reloads and agreed that
>>>
>> this is the
>>
>>> safest approach, albeit with a bit of sacrifice in consistent
>>>
>> behavior.
>>
>> That's right.  We actually had a previous fix for this that preserved
>> the forward history, but it led to deeper conflicts with page IDs that
>> caused renderer crashes.  There's more history on http://crbug.com/80621
>> if you're interested.
>>
>>
https://chromiumcodereview.**appspot.com/9169065/<https://chromiumcodereview....
>>
>
>

[darin (slow to review), 2012-01-28 05:01:41]

On Fri, Jan 27, 2012 at 5:43 PM, <nasko@chromium.org> wrote:

> Responses to Darin's comments.
>
>
>
> https://chromiumcodereview.**appspot.com/9169065/diff/**
>
11001/chrome/renderer/chrome_**content_renderer_client.cc<https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chrome_content_renderer_client.cc>
> File chrome/renderer/chrome_**content_renderer_client.cc (right):
>
> https://chromiumcodereview.**appspot.com/9169065/diff/**
>
11001/chrome/renderer/chrome_**content_renderer_client.cc#**newcode763<https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chrome_content_renderer_client.cc#newcode763>
> chrome/renderer/chrome_**content_renderer_client.cc:**763: if (old_url ==
> new_url) {
> On 2012/01/27 23:58:35, darin wrote:
>
>> On 2012/01/27 20:14:56, nasko wrote:
>> > On 2012/01/27 19:34:11, darin wrote:
>> > > This condition isn't necessarily limited to a reload.  You can
>>
> also get this
>
>> > by
>> > > hitting enter in the location bar.
>> > >
>> > > (Generally, reload is reserved to correspond to clicking the
>>
> "reload"
>
>> button.)
>> > >
>> > > Also, are you sure comparing URLs is enough?  Maybe we should be
>>
> comparing
>
>> > site
>> > > instances?  What about if the URLs are the same but only differ in
>>
> terms of
>
>> > > their reference fragment?
>> > >
>> > > Also, isn't this abusing the meaning of CrossesExtensionExtents?
>>
> It seems
>
>> > like
>> > > if the before and after URL are identical, then it cannot possibly
>>
> cross
>
>> > > extension extents.  Is this function misnamed?  Given the current
>>
> name, I
>
>> > > wouldn't be surprised if the caller performed an URL equivalency
>>
> test as a
>
>> > > short-cut around having to call this function.
>>
>
>  ^^^ what about this last point?  is this really the right location for
>>
> this?
>
>
>  > Hitting enter in the location bar is handled by the browser process,
>>
> as well
>
>> as
>> > clicking on the reload button. This class is running in the renderer
>>
> process,
>
>> so
>> > it doesn't come into picture for those two actions.
>> >
>> > Site instance data is also not available in the renderer process, so
>>
> we can't
>
>> > really compare them. This code is added to handle renderer initiated
>>
> reloads,
>
>> > one instance of which is dong location.reload() in JS. I've
>>
> discussed with
>
>> > Charlie that we need better way to express privilege level for
>>
> renderer
>
>> > processes, so they can make decision whether to route top level
>>
> navigation
>
>> > events to the browser or not. Until we do this work, we can handle
>>
> JS reload
>
>> > here. So yes, I agree this can be much better, but it is a much
>>
> bigger effort,
>
>> > belonging to a separate issue.
>>
>
>
> It could be moved to the function that calls this, which is named more
> appropriately (ShouldFork), though we are indeed crossing a boundary,
> which involves time. If you believe it is better in the caller, I'd be
> happy to move it. I'm still trying to learn the componentization and how
> things fit together : ).
>
>
Yes, ChromeContentRendererClient::ShouldFork seems like a much
better place for this.



> https://chromiumcodereview.**appspot.com/9169065/diff/**
>
11001/chrome/renderer/chrome_**content_renderer_client.cc#**newcode766<https://chromiumcodereview.appspot.com/9169065/diff/11001/chrome/renderer/chrome_content_renderer_client.cc#newcode766>
> chrome/renderer/chrome_**content_renderer_client.cc:**766: return true;
> On 2012/01/27 23:58:35, darin wrote:
>
>> what about the reverse case?  what if the app is uninstalled and the
>>
> reloaded?
>
> Yes, if the process "type" mismatches with the current expectation, it
> will detect it and route to the browser.
>
>
Cool.


>
https://chromiumcodereview.**appspot.com/9169065/<https://chromiumcodereview....
>

[nasko, 2012-01-30 21:20:49]

The latest patch moves the logic to detect javascript reload to the ShouldFork
function.

[darin (slow to review), 2012-01-30 22:14:47]

LGTM.  Just one nit:

https://chromiumcodereview.appspot.com/9169065/diff/14001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/14001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:657: // should send it to the
browser if it's an extension URL (e.g., hosted app)
nit: hosted apps don't use extension URLs, so I'm a bit confused by this
comment.

[Charlie Reis, 2012-01-30 22:21:56]

https://chromiumcodereview.appspot.com/9169065/diff/14001/chrome/renderer/chr...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://chromiumcodereview.appspot.com/9169065/diff/14001/chrome/renderer/chr...
chrome/renderer/chrome_content_renderer_client.cc:657: // should send it to the
browser if it's an extension URL (e.g., hosted app)
On 2012/01/30 22:14:48, darin wrote:
> nit: hosted apps don't use extension URLs, so I'm a bit confused by this
> comment.

The effective URL of a hosted app is actually an extension URL, which is how we
notice them in the process model.  (SiteInstanceImpl::GetEffectiveURL)

[darin (slow to review), 2012-01-30 22:46:54]

On Mon, Jan 30, 2012 at 2:21 PM, <creis@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/9169065/diff/**
>
14001/chrome/renderer/chrome_**content_renderer_client.cc<https://chromiumcodereview.appspot.com/9169065/diff/14001/chrome/renderer/chrome_content_renderer_client.cc>
> File chrome/renderer/chrome_**content_renderer_client.cc (right):
>
> https://chromiumcodereview.**appspot.com/9169065/diff/**
>
14001/chrome/renderer/chrome_**content_renderer_client.cc#**newcode657<https://chromiumcodereview.appspot.com/9169065/diff/14001/chrome/renderer/chrome_content_renderer_client.cc#newcode657>
> chrome/renderer/chrome_**content_renderer_client.cc:**657: // should send
> it
> to the browser if it's an extension URL (e.g., hosted app)
> On 2012/01/30 22:14:48, darin wrote:
>
>> nit: hosted apps don't use extension URLs, so I'm a bit confused by
>>
> this
>
>> comment.
>>
>
> The effective URL of a hosted app is actually an extension URL, which is
> how we notice them in the process model.
> (SiteInstanceImpl::**GetEffectiveURL)
>
>
Ah!!!  OK :)


>
https://chromiumcodereview.**appspot.com/9169065/<https://chromiumcodereview....
>

[commit-bot: I haz the power, 2012-01-31 19:07:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/9169065/14001

[commit-bot: I haz the power, 2012-01-31 19:07:24]

Can't apply patch for file
content/browser/tab_contents/navigation_controller_impl.cc.
While running patch -p1 --forward --force;
patching file content/browser/tab_contents/navigation_controller_impl.cc
Hunk #1 FAILED at 266.
Hunk #2 succeeded at 590 (offset 7 lines).
1 out of 2 hunks FAILED -- saving rejects to file
content/browser/tab_contents/navigation_controller_impl.cc.rej

[nasko, 2012-01-31 22:50:55]

Fixed a merge conflict.

Darin, can you submit the change to the commit queue? Charlie hit the merge
conflict and is not around for the afternoon.

[commit-bot: I haz the power, 2012-01-31 23:32:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/9169065/23001

[commit-bot: I haz the power, 2012-02-01 01:54:25]

Change committed as 120007