Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2972793002:
Fix did_replace_entry reported as false for meta refresh tags (Closed)
DescriptionFix did_replace_entry reported as false for meta refresh tags
The renderer already reports whether the navigation replaced the
previous one, so we shouldn't ignore that.
This change is needed after https://codereview.chromium.org/1214073005
introduced the override for NAVIGATION_TYPE_EXISTING_PAGE as an attempt
to replace the former (now ancient) NAVIGATION_TYPE_IN_PAGE with
NAVIGATION_TYPE_EXISTING_PAGE + is_in_page.
BUG=498436
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Patch Set 1 #
Total comments: 1
Patch Set 2 : Minor rewrite and updated tests. #
Dependent Patchsets: Messages
Total messages: 24 (12 generated)
Description was changed from ========== Fix did_replace_entry reported as false for meta refresh tags The renderer already reports whether the navigation replaced the previous one, so we shouldn't ignore that. This change is needed after https://codereview.chromium.org/1214073005 introduced the override for NAVIGATION_TYPE_EXISTING_PAGE as an attempt to replace the former (now ancient) NAVIGATION_TYPE_IN_PAGE with NAVIGATION_TYPE_EXISTING_PAGE + is_in_page. BUG=498436 ========== to ========== Fix did_replace_entry reported as false for meta refresh tags The renderer already reports whether the navigation replaced the previous one, so we shouldn't ignore that. This change is needed after https://codereview.chromium.org/1214073005 introduced the override for NAVIGATION_TYPE_EXISTING_PAGE as an attempt to replace the former (now ancient) NAVIGATION_TYPE_IN_PAGE with NAVIGATION_TYPE_EXISTING_PAGE + is_in_page. BUG=498436 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mastiz@chromium.org changed reviewers: + avi@chromium.org
avi@: Does a change of this kind make sense to you? I came across this weirdness when debugging an issue with favicons (linked in the patch). It doesn't solve the whole problem but it seems a step forward. I'm also not aware of all the side effects of this change.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
On 2017/07/05 11:23:08, mastiz wrote: > avi@: Does a change of this kind make sense to you? I came across this weirdness > when debugging an issue with favicons (linked in the patch). It doesn't solve > the whole problem but it seems a step forward. I'm also not aware of all the > side effects of this change. There seem to be a few tests specifically verifying such behavior: https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... I'd be interested in knowing the rationale behind this logic.
Description was changed from ========== Fix did_replace_entry reported as false for meta refresh tags The renderer already reports whether the navigation replaced the previous one, so we shouldn't ignore that. This change is needed after https://codereview.chromium.org/1214073005 introduced the override for NAVIGATION_TYPE_EXISTING_PAGE as an attempt to replace the former (now ancient) NAVIGATION_TYPE_IN_PAGE with NAVIGATION_TYPE_EXISTING_PAGE + is_in_page. BUG=498436 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix did_replace_entry reported as false for meta refresh tags The renderer already reports whether the navigation replaced the previous one, so we shouldn't ignore that. This change is needed after https://codereview.chromium.org/1214073005 introduced the override for NAVIGATION_TYPE_EXISTING_PAGE as an attempt to replace the former (now ancient) NAVIGATION_TYPE_IN_PAGE with NAVIGATION_TYPE_EXISTING_PAGE + is_in_page. BUG=498436 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
avi@chromium.org changed reviewers: + creis@chromium.org, jam@chromium.org
On 2017/07/05 13:16:35, mastiz wrote: > On 2017/07/05 11:23:08, mastiz wrote: > > avi@: Does a change of this kind make sense to you? I came across this > weirdness > > when debugging an issue with favicons (linked in the patch). It doesn't solve > > the whole problem but it seems a step forward. I'm also not aware of all the > > side effects of this change. > > There seem to be a few tests specifically verifying such behavior: > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > I'd be interested in knowing the rationale behind this logic. +jam who added this test in https://chromiumcodereview.appspot.com/10915002, although it was a port of a pyauto test, he might know +charlie who is also into nav I'm not personally attached to the behavior of meta http-equiv="refresh" bring treated as a new page rather than a redirect but since we've had it so long this way, changing it would be web-visible. Does a relevant spec have any guidance here? How do other browsers treat it? Changing the behavior may well require an Intent. :(
On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > On 2017/07/05 13:16:35, mastiz wrote: > > On 2017/07/05 11:23:08, mastiz wrote: > > > avi@: Does a change of this kind make sense to you? I came across this > > weirdness > > > when debugging an issue with favicons (linked in the patch). It doesn't > solve > > > the whole problem but it seems a step forward. I'm also not aware of all the > > > side effects of this change. > > > > There seem to be a few tests specifically verifying such behavior: > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > I'd be interested in knowing the rationale behind this logic. > > +jam who added this test in https://chromiumcodereview.appspot.com/10915002, > although it was a port of a pyauto test, he might know Sorry, not really knowledgable in this area. I defer to Charlie. > +charlie who is also into nav > > I'm not personally attached to the behavior of meta http-equiv="refresh" bring > treated as a new page rather than a redirect but since we've had it so long this > way, changing it would be web-visible. Does a relevant spec have any guidance > here? How do other browsers treat it? Changing the behavior may well require an > Intent. :(
On 2017/07/06 15:03:19, jam wrote: > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > On 2017/07/05 13:16:35, mastiz wrote: > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > avi@: Does a change of this kind make sense to you? I came across this > > > weirdness > > > > when debugging an issue with favicons (linked in the patch). It doesn't > > solve > > > > the whole problem but it seems a step forward. I'm also not aware of all > the > > > > side effects of this change. > > > > > > There seem to be a few tests specifically verifying such behavior: > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > +jam who added this test in https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > although it was a port of a pyauto test, he might know > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > +charlie who is also into nav > > > > I'm not personally attached to the behavior of meta http-equiv="refresh" bring > > treated as a new page rather than a redirect but since we've had it so long > this > > way, changing it would be web-visible. Does a relevant spec have any guidance > > here? How do other browsers treat it? Changing the behavior may well require > an > > Intent. :( This is really interesting. At first, it looks like a clear regression added in Avi's CL, where is_same_document should make us set did_replace_entry to true but never force it to false. But if that's the case, why is an even older test failing with the change? (How would it have passed before Avi's CL?) Maybe it has something to do with how we used to pull did_replace_entry from the pending_entry_ (at the time Avi's CL landed) but now we let the renderer tell us (via should_replace_current_entry)? Maybe that change would have caused the test to fail if not for this other bug? I'll point out that we don't seem to leave a NavigationEntry around for redirector.html with or without this CL. The change is only visible to observers of the navigation, like the Chrome history database in the test. So maybe it's not such a disruptive change, and we could even update the history code if needed. Still, I'm having trouble understanding the code itself (as noted below), and why is_same_document implies did_replace_entry for EXISTING_PAGE navigations. In fact, it's a bit weird to me that this is considered EXISTING_PAGE in the first place-- it feels kind of like https://crbug.com/596707, where it should be classified as NEW_PAGE with replacement rather than EXISTING_PAGE. I wonder if that's a better way to handle this, and if the history test would be happy in that case? I don't have more time to investigate this today, but we probably should get to the bottom of this. https://codereview.chromium.org/2972793002/diff/1/content/browser/frame_host/... File content/browser/frame_host/navigation_controller_impl.cc (right): https://codereview.chromium.org/2972793002/diff/1/content/browser/frame_host/... content/browser/frame_host/navigation_controller_impl.cc:892: details->did_replace_entry || details->is_same_document; Might be cleaner to rephrase it as: if (details->is_same_document) details->did_replace_entry = true; or even: details->did_replace_entry |= details->is_same_document; Either way, it's worth having a comment saying why is_same_document implies that it replaced the entry. I'm having trouble understanding why that's the case, though (even before Avi's CL, when it was something about the page ID not changing).
Thanks for the detailed reply. On 2017/07/07 22:52:55, Charlie Reis (slow) wrote: > On 2017/07/06 15:03:19, jam wrote: > > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > > On 2017/07/05 13:16:35, mastiz wrote: > > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > > avi@: Does a change of this kind make sense to you? I came across this > > > > weirdness > > > > > when debugging an issue with favicons (linked in the patch). It doesn't > > > solve > > > > > the whole problem but it seems a step forward. I'm also not aware of all > > the > > > > > side effects of this change. > > > > > > > > There seem to be a few tests specifically verifying such behavior: > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > > > +jam who added this test in > https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > > although it was a port of a pyauto test, he might know > > > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > > > +charlie who is also into nav > > > > > > I'm not personally attached to the behavior of meta http-equiv="refresh" > bring > > > treated as a new page rather than a redirect but since we've had it so long > > this > > > way, changing it would be web-visible. Does a relevant spec have any > guidance > > > here? How do other browsers treat it? Changing the behavior may well require > > an > > > Intent. :( > > This is really interesting. At first, it looks like a clear regression added in > Avi's CL, where is_same_document should make us set did_replace_entry to true > but never force it to false. But if that's the case, why is an even older test > failing with the change? (How would it have passed before Avi's CL?) I think the > > Maybe it has something to do with how we used to pull did_replace_entry from the > pending_entry_ (at the time Avi's CL landed) but now we let the renderer tell us > (via should_replace_current_entry)? Maybe that change would have caused the > test to fail if not for this other bug? > > I'll point out that we don't seem to leave a NavigationEntry around for > redirector.html with or without this CL. The change is only visible to > observers of the navigation, like the Chrome history database in the test. So > maybe it's not such a disruptive change, and we could even update the history > code if needed. > > Still, I'm having trouble understanding the code itself (as noted below), and > why is_same_document implies did_replace_entry for EXISTING_PAGE navigations. > In fact, it's a bit weird to me that this is considered EXISTING_PAGE in the > first place-- it feels kind of like https://crbug.com/596707, where it should be > classified as NEW_PAGE with replacement rather than EXISTING_PAGE. I wonder if > that's a better way to handle this, and if the history test would be happy in > that case? > > I don't have more time to investigate this today, but we probably should get to > the bottom of this. Based on the documentation for NAVIGATION_TYPE_NEW_PAGE, it does seem like it's a better fit. avi@, do you recall why EXISTING_PAGE was chosen instead? I can give this a try but my knowledge here is very limited so I'd appreciate someone else being involved. Thanks, Mikel > > https://codereview.chromium.org/2972793002/diff/1/content/browser/frame_host/... > File content/browser/frame_host/navigation_controller_impl.cc (right): > > https://codereview.chromium.org/2972793002/diff/1/content/browser/frame_host/... > content/browser/frame_host/navigation_controller_impl.cc:892: > details->did_replace_entry || details->is_same_document; > Might be cleaner to rephrase it as: > > if (details->is_same_document) > details->did_replace_entry = true; > > or even: > details->did_replace_entry |= details->is_same_document; > > Either way, it's worth having a comment saying why is_same_document implies that > it replaced the entry. I'm having trouble understanding why that's the case, > though (even before Avi's CL, when it was something about the page ID not > changing). Just to clarify
On 2017/07/10 18:53:17, mastiz wrote: > Thanks for the detailed reply. > > On 2017/07/07 22:52:55, Charlie Reis (slow) wrote: > > On 2017/07/06 15:03:19, jam wrote: > > > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > > > On 2017/07/05 13:16:35, mastiz wrote: > > > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > > > avi@: Does a change of this kind make sense to you? I came across this > > > > > weirdness > > > > > > when debugging an issue with favicons (linked in the patch). It > doesn't > > > > solve > > > > > > the whole problem but it seems a step forward. I'm also not aware of > all > > > the > > > > > > side effects of this change. > > > > > > > > > > There seem to be a few tests specifically verifying such behavior: > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > > > > > +jam who added this test in > > > https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > > > although it was a port of a pyauto test, he might know > > > > > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > > > > > +charlie who is also into nav > > > > > > > > I'm not personally attached to the behavior of meta http-equiv="refresh" > > bring > > > > treated as a new page rather than a redirect but since we've had it so > long > > > this > > > > way, changing it would be web-visible. Does a relevant spec have any > > guidance > > > > here? How do other browsers treat it? Changing the behavior may well > require > > > an > > > > Intent. :( > > > > This is really interesting. At first, it looks like a clear regression added > in > > Avi's CL, where is_same_document should make us set did_replace_entry to true > > but never force it to false. But if that's the case, why is an even older > test > > failing with the change? (How would it have passed before Avi's CL?) > > I think the > > > > > Maybe it has something to do with how we used to pull did_replace_entry from > the > > pending_entry_ (at the time Avi's CL landed) but now we let the renderer tell > us > > (via should_replace_current_entry)? Maybe that change would have caused the > > test to fail if not for this other bug? > > > > I'll point out that we don't seem to leave a NavigationEntry around for > > redirector.html with or without this CL. The change is only visible to > > observers of the navigation, like the Chrome history database in the test. So > > maybe it's not such a disruptive change, and we could even update the history > > code if needed. > > > > Still, I'm having trouble understanding the code itself (as noted below), and > > why is_same_document implies did_replace_entry for EXISTING_PAGE navigations. > > In fact, it's a bit weird to me that this is considered EXISTING_PAGE in the > > first place-- it feels kind of like https://crbug.com/596707, where it should > be > > classified as NEW_PAGE with replacement rather than EXISTING_PAGE. I wonder > if Would the above involve both did_create_new_entry==true and should_replace_current_entry? Is that not a contradiction? In general, I realized the implications of https://crbug.com/596707 are more complex than I first thought. creis@, is this something you plan to address? Thanks, Mikel > > that's a better way to handle this, and if the history test would be happy in > > that case? > > > > I don't have more time to investigate this today, but we probably should get > to > > the bottom of this. > > Based on the documentation for NAVIGATION_TYPE_NEW_PAGE, it does seem like it's > a better fit. avi@, do you recall why EXISTING_PAGE was chosen instead? > > I can give this a try but my knowledge here is very limited so I'd appreciate > someone else being involved. > > Thanks, > Mikel > > > > > > https://codereview.chromium.org/2972793002/diff/1/content/browser/frame_host/... > > File content/browser/frame_host/navigation_controller_impl.cc (right): > > > > > https://codereview.chromium.org/2972793002/diff/1/content/browser/frame_host/... > > content/browser/frame_host/navigation_controller_impl.cc:892: > > details->did_replace_entry || details->is_same_document; > > Might be cleaner to rephrase it as: > > > > if (details->is_same_document) > > details->did_replace_entry = true; > > > > or even: > > details->did_replace_entry |= details->is_same_document; > > > > Either way, it's worth having a comment saying why is_same_document implies > that > > it replaced the entry. I'm having trouble understanding why that's the case, > > though (even before Avi's CL, when it was something about the page ID not > > changing). > > Just to clarify
On 2017/07/11 19:08:59, mastiz wrote: > On 2017/07/10 18:53:17, mastiz wrote: > > Thanks for the detailed reply. > > > > On 2017/07/07 22:52:55, Charlie Reis (slow) wrote: > > > On 2017/07/06 15:03:19, jam wrote: > > > > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > > > > On 2017/07/05 13:16:35, mastiz wrote: > > > > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > > > > avi@: Does a change of this kind make sense to you? I came across > this > > > > > > weirdness > > > > > > > when debugging an issue with favicons (linked in the patch). It > > doesn't > > > > > solve > > > > > > > the whole problem but it seems a step forward. I'm also not aware of > > all > > > > the > > > > > > > side effects of this change. > > > > > > > > > > > > There seem to be a few tests specifically verifying such behavior: > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > > > > > > > +jam who added this test in > > > > > > https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > > > > although it was a port of a pyauto test, he might know > > > > > > > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > > > > > > > +charlie who is also into nav > > > > > > > > > > I'm not personally attached to the behavior of meta http-equiv="refresh" > > > bring > > > > > treated as a new page rather than a redirect but since we've had it so > > long > > > > this > > > > > way, changing it would be web-visible. Does a relevant spec have any > > > guidance > > > > > here? How do other browsers treat it? Changing the behavior may well > > require > > > > an > > > > > Intent. :( > > > > > > This is really interesting. At first, it looks like a clear regression > added > > in > > > Avi's CL, where is_same_document should make us set did_replace_entry to > true > > > but never force it to false. But if that's the case, why is an even older > > test > > > failing with the change? (How would it have passed before Avi's CL?) > > > > I think the > > > > > > > > Maybe it has something to do with how we used to pull did_replace_entry from > > the > > > pending_entry_ (at the time Avi's CL landed) but now we let the renderer > tell > > us > > > (via should_replace_current_entry)? Maybe that change would have caused the > > > test to fail if not for this other bug? > > > > > > I'll point out that we don't seem to leave a NavigationEntry around for > > > redirector.html with or without this CL. The change is only visible to > > > observers of the navigation, like the Chrome history database in the test. > So > > > maybe it's not such a disruptive change, and we could even update the > history > > > code if needed. > > > > > > Still, I'm having trouble understanding the code itself (as noted below), > and > > > why is_same_document implies did_replace_entry for EXISTING_PAGE > navigations. > > > In fact, it's a bit weird to me that this is considered EXISTING_PAGE in the > > > first place-- it feels kind of like https://crbug.com/596707, where it > should > > be > > > classified as NEW_PAGE with replacement rather than EXISTING_PAGE. I wonder > > if > > Would the above involve both did_create_new_entry==true and > should_replace_current_entry? Is that not a contradiction? > > In general, I realized the implications of https://crbug.com/596707 are more > complex than I first thought. creis@, is this something you plan to address? > > Thanks, > Mikel It's not a contradiction-- we can and should create a new NavigationEntry (i.e., did_create_new_entry) which replaces the current one (i.e., should_replace_current_entry). We should be doing that for location.replace and probably this meta refresh case as well, since these navigations can be cross-site and cross-process, and we shouldn't just try to update the existing NavigationEntry (as we would for something like history.replaceState). Yes, this is an involved change, and I've been putting it off. I can try to spend some time on it, though. Can you help me understand one aspect of this? How did this bug relate to the favicon issue in https://crbug.com/498436? Did details->did_replace_entry == false affect something in the favicon code? (Just trying to close the loop to understand why it's required, and to make sure we can write tests as appropriate.)
On 2017/07/11 22:07:05, Charlie Reis (slow) wrote: > On 2017/07/11 19:08:59, mastiz wrote: > > On 2017/07/10 18:53:17, mastiz wrote: > > > Thanks for the detailed reply. > > > > > > On 2017/07/07 22:52:55, Charlie Reis (slow) wrote: > > > > On 2017/07/06 15:03:19, jam wrote: > > > > > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > > > > > On 2017/07/05 13:16:35, mastiz wrote: > > > > > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > > > > > avi@: Does a change of this kind make sense to you? I came across > > this > > > > > > > weirdness > > > > > > > > when debugging an issue with favicons (linked in the patch). It > > > doesn't > > > > > > solve > > > > > > > > the whole problem but it seems a step forward. I'm also not aware > of > > > all > > > > > the > > > > > > > > side effects of this change. > > > > > > > > > > > > > > There seem to be a few tests specifically verifying such behavior: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > > > > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > > > > > > > > > +jam who added this test in > > > > > > > > > > https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > > > > > although it was a port of a pyauto test, he might know > > > > > > > > > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > > > > > > > > > +charlie who is also into nav > > > > > > > > > > > > I'm not personally attached to the behavior of meta > http-equiv="refresh" > > > > bring > > > > > > treated as a new page rather than a redirect but since we've had it so > > > long > > > > > this > > > > > > way, changing it would be web-visible. Does a relevant spec have any > > > > guidance > > > > > > here? How do other browsers treat it? Changing the behavior may well > > > require > > > > > an > > > > > > Intent. :( > > > > > > > > This is really interesting. At first, it looks like a clear regression > > added > > > in > > > > Avi's CL, where is_same_document should make us set did_replace_entry to > > true > > > > but never force it to false. But if that's the case, why is an even older > > > test > > > > failing with the change? (How would it have passed before Avi's CL?) > > > > > > I think the > > > > > > > > > > > Maybe it has something to do with how we used to pull did_replace_entry > from > > > the > > > > pending_entry_ (at the time Avi's CL landed) but now we let the renderer > > tell > > > us > > > > (via should_replace_current_entry)? Maybe that change would have caused > the > > > > test to fail if not for this other bug? > > > > > > > > I'll point out that we don't seem to leave a NavigationEntry around for > > > > redirector.html with or without this CL. The change is only visible to > > > > observers of the navigation, like the Chrome history database in the test. > > > So > > > > maybe it's not such a disruptive change, and we could even update the > > history > > > > code if needed. > > > > > > > > Still, I'm having trouble understanding the code itself (as noted below), > > and > > > > why is_same_document implies did_replace_entry for EXISTING_PAGE > > navigations. > > > > In fact, it's a bit weird to me that this is considered EXISTING_PAGE in > the > > > > first place-- it feels kind of like https://crbug.com/596707, where it > > should > > > be > > > > classified as NEW_PAGE with replacement rather than EXISTING_PAGE. I > wonder > > > if > > > > Would the above involve both did_create_new_entry==true and > > should_replace_current_entry? Is that not a contradiction? > > > > In general, I realized the implications of https://crbug.com/596707 are more > > complex than I first thought. creis@, is this something you plan to address? > > > > Thanks, > > Mikel > > It's not a contradiction-- we can and should create a new NavigationEntry (i.e., > did_create_new_entry) which replaces the current one (i.e., > should_replace_current_entry). We should be doing that for location.replace and > probably this meta refresh case as well, since these navigations can be > cross-site and cross-process, and we shouldn't just try to update the existing > NavigationEntry (as we would for something like history.replaceState). > > Yes, this is an involved change, and I've been putting it off. I can try to > spend some time on it, though. > > Can you help me understand one aspect of this? How did this bug relate to the > favicon issue in https://crbug.com/498436 Did details->did_replace_entry == > false affect something in the favicon code? (Just trying to close the loop to > understand why it's required, and to make sure we can write tests as > appropriate.) https://crbug.com/498436 doesn't affect favicon logic (AFAIK) as per today. I'm trying to improve favicon logic (https://codereview.chromium.org/2949023002/) to better handle client-side redirects when I realized it was hard to tell some cases apart (in particular meta refresh tags to different pages, because did_replace_entry==false). I'm assuming did_replace_entry would be exposed as true to upper layers after your change, which would apparently break HistoryBrowserTest.RedirectHistory because the refresh delay there is 0 (<=1). If you don't plan to change the behavior of did_replace_entry despite migrating to NEW_PAGE, I'd need to continue working on this bug. Thanks.
The CQ bit was checked by mastiz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
On 2017/07/12 05:15:40, mastiz wrote: > On 2017/07/11 22:07:05, Charlie Reis (slow) wrote: > > On 2017/07/11 19:08:59, mastiz wrote: > > > On 2017/07/10 18:53:17, mastiz wrote: > > > > Thanks for the detailed reply. > > > > > > > > On 2017/07/07 22:52:55, Charlie Reis (slow) wrote: > > > > > On 2017/07/06 15:03:19, jam wrote: > > > > > > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > > > > > > On 2017/07/05 13:16:35, mastiz wrote: > > > > > > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > > > > > > avi@: Does a change of this kind make sense to you? I came > across > > > this > > > > > > > > weirdness > > > > > > > > > when debugging an issue with favicons (linked in the patch). It > > > > doesn't > > > > > > > solve > > > > > > > > > the whole problem but it seems a step forward. I'm also not > aware > > of > > > > all > > > > > > the > > > > > > > > > side effects of this change. > > > > > > > > > > > > > > > > There seem to be a few tests specifically verifying such behavior: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > > > > > > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > > > > > > > > > > > +jam who added this test in > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > > > > > > although it was a port of a pyauto test, he might know > > > > > > > > > > > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > > > > > > > > > > > +charlie who is also into nav > > > > > > > > > > > > > > I'm not personally attached to the behavior of meta > > http-equiv="refresh" > > > > > bring > > > > > > > treated as a new page rather than a redirect but since we've had it > so > > > > long > > > > > > this > > > > > > > way, changing it would be web-visible. Does a relevant spec have any > > > > > guidance > > > > > > > here? How do other browsers treat it? Changing the behavior may well > > > > require > > > > > > an > > > > > > > Intent. :( > > > > > > > > > > This is really interesting. At first, it looks like a clear regression > > > added > > > > in > > > > > Avi's CL, where is_same_document should make us set did_replace_entry to > > > true > > > > > but never force it to false. But if that's the case, why is an even > older > > > > test > > > > > failing with the change? (How would it have passed before Avi's CL?) > > > > > > > > I think the > > > > > > > > > > > > > > Maybe it has something to do with how we used to pull did_replace_entry > > from > > > > the > > > > > pending_entry_ (at the time Avi's CL landed) but now we let the renderer > > > tell > > > > us > > > > > (via should_replace_current_entry)? Maybe that change would have caused > > the > > > > > test to fail if not for this other bug? > > > > > > > > > > I'll point out that we don't seem to leave a NavigationEntry around for > > > > > redirector.html with or without this CL. The change is only visible to > > > > > observers of the navigation, like the Chrome history database in the > test. > > > > > So > > > > > maybe it's not such a disruptive change, and we could even update the > > > history > > > > > code if needed. > > > > > > > > > > Still, I'm having trouble understanding the code itself (as noted > below), > > > and > > > > > why is_same_document implies did_replace_entry for EXISTING_PAGE > > > navigations. > > > > > In fact, it's a bit weird to me that this is considered EXISTING_PAGE in > > the > > > > > first place-- it feels kind of like https://crbug.com/596707, where it > > > should > > > > be > > > > > classified as NEW_PAGE with replacement rather than EXISTING_PAGE. I > > wonder > > > > if > > > > > > Would the above involve both did_create_new_entry==true and > > > should_replace_current_entry? Is that not a contradiction? > > > > > > In general, I realized the implications of https://crbug.com/596707 are more > > > complex than I first thought. creis@, is this something you plan to address? > > > > > > Thanks, > > > Mikel > > > > It's not a contradiction-- we can and should create a new NavigationEntry > (i.e., > > did_create_new_entry) which replaces the current one (i.e., > > should_replace_current_entry). We should be doing that for location.replace > and > > probably this meta refresh case as well, since these navigations can be > > cross-site and cross-process, and we shouldn't just try to update the existing > > NavigationEntry (as we would for something like history.replaceState). > > > > Yes, this is an involved change, and I've been putting it off. I can try to > > spend some time on it, though. > > > > Can you help me understand one aspect of this? How did this bug relate to the > > favicon issue in https://crbug.com/498436 Did details->did_replace_entry == > > false affect something in the favicon code? (Just trying to close the loop to > > understand why it's required, and to make sure we can write tests as > > appropriate.) > > https://crbug.com/498436 doesn't affect favicon logic (AFAIK) as per today. I'm > trying to improve favicon logic (https://codereview.chromium.org/2949023002/) to > better handle client-side redirects when I realized it was hard to tell some > cases apart (in particular meta refresh tags to different pages, because > did_replace_entry==false). I'm assuming did_replace_entry would be exposed as > true to upper layers after your change, which would apparently break > HistoryBrowserTest.RedirectHistory because the refresh delay there is 0 (<=1). > > If you don't plan to change the behavior of did_replace_entry despite migrating > to NEW_PAGE, I'd need to continue working on this bug. Thanks. Further investigation suggests this bug is not on my way and actually meta refresh tags are propagated well via page transition type modifiers. So I could be off track with this change, although it does seem confusing to have two subtly different semantics for did_replace_entry.
On 2017/07/12 13:30:15, mastiz wrote: > On 2017/07/12 05:15:40, mastiz wrote: > > On 2017/07/11 22:07:05, Charlie Reis (slow) wrote: > > > On 2017/07/11 19:08:59, mastiz wrote: > > > > On 2017/07/10 18:53:17, mastiz wrote: > > > > > Thanks for the detailed reply. > > > > > > > > > > On 2017/07/07 22:52:55, Charlie Reis (slow) wrote: > > > > > > On 2017/07/06 15:03:19, jam wrote: > > > > > > > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > > > > > > > On 2017/07/05 13:16:35, mastiz wrote: > > > > > > > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > > > > > > > avi@: Does a change of this kind make sense to you? I came > > across > > > > this > > > > > > > > > weirdness > > > > > > > > > > when debugging an issue with favicons (linked in the patch). > It > > > > > doesn't > > > > > > > > solve > > > > > > > > > > the whole problem but it seems a step forward. I'm also not > > aware > > > of > > > > > all > > > > > > > the > > > > > > > > > > side effects of this change. > > > > > > > > > > > > > > > > > > There seem to be a few tests specifically verifying such > behavior: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > > > > > > > > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > > > > > > > > > > > > > +jam who added this test in > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > > > > > > > although it was a port of a pyauto test, he might know > > > > > > > > > > > > > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > > > > > > > > > > > > > +charlie who is also into nav > > > > > > > > > > > > > > > > I'm not personally attached to the behavior of meta > > > http-equiv="refresh" > > > > > > bring > > > > > > > > treated as a new page rather than a redirect but since we've had > it > > so > > > > > long > > > > > > > this > > > > > > > > way, changing it would be web-visible. Does a relevant spec have > any > > > > > > guidance > > > > > > > > here? How do other browsers treat it? Changing the behavior may > well > > > > > require > > > > > > > an > > > > > > > > Intent. :( > > > > > > > > > > > > This is really interesting. At first, it looks like a clear > regression > > > > added > > > > > in > > > > > > Avi's CL, where is_same_document should make us set did_replace_entry > to > > > > true > > > > > > but never force it to false. But if that's the case, why is an even > > older > > > > > test > > > > > > failing with the change? (How would it have passed before Avi's CL?) > > > > > > > > > > I think the > > > > > > > > > > > > > > > > > Maybe it has something to do with how we used to pull > did_replace_entry > > > from > > > > > the > > > > > > pending_entry_ (at the time Avi's CL landed) but now we let the > renderer > > > > tell > > > > > us > > > > > > (via should_replace_current_entry)? Maybe that change would have > caused > > > the > > > > > > test to fail if not for this other bug? > > > > > > > > > > > > I'll point out that we don't seem to leave a NavigationEntry around > for > > > > > > redirector.html with or without this CL. The change is only visible > to > > > > > > observers of the navigation, like the Chrome history database in the > > test. > > > > > > > So > > > > > > maybe it's not such a disruptive change, and we could even update the > > > > history > > > > > > code if needed. > > > > > > > > > > > > Still, I'm having trouble understanding the code itself (as noted > > below), > > > > and > > > > > > why is_same_document implies did_replace_entry for EXISTING_PAGE > > > > navigations. > > > > > > In fact, it's a bit weird to me that this is considered EXISTING_PAGE > in > > > the > > > > > > first place-- it feels kind of like https://crbug.com/596707, where it > > > > should > > > > > be > > > > > > classified as NEW_PAGE with replacement rather than EXISTING_PAGE. I > > > wonder > > > > > if > > > > > > > > Would the above involve both did_create_new_entry==true and > > > > should_replace_current_entry? Is that not a contradiction? > > > > > > > > In general, I realized the implications of https://crbug.com/596707 are > more > > > > complex than I first thought. creis@, is this something you plan to > address? > > > > > > > > Thanks, > > > > Mikel > > > > > > It's not a contradiction-- we can and should create a new NavigationEntry > > (i.e., > > > did_create_new_entry) which replaces the current one (i.e., > > > should_replace_current_entry). We should be doing that for location.replace > > and > > > probably this meta refresh case as well, since these navigations can be > > > cross-site and cross-process, and we shouldn't just try to update the > existing > > > NavigationEntry (as we would for something like history.replaceState). > > > > > > Yes, this is an involved change, and I've been putting it off. I can try to > > > spend some time on it, though. > > > > > > Can you help me understand one aspect of this? How did this bug relate to > the > > > favicon issue in https://crbug.com/498436 Did details->did_replace_entry == > > > false affect something in the favicon code? (Just trying to close the loop > to > > > understand why it's required, and to make sure we can write tests as > > > appropriate.) > > > > https://crbug.com/498436 doesn't affect favicon logic (AFAIK) as per today. > I'm > > trying to improve favicon logic (https://codereview.chromium.org/2949023002/) > to > > better handle client-side redirects when I realized it was hard to tell some > > cases apart (in particular meta refresh tags to different pages, because > > did_replace_entry==false). I'm assuming did_replace_entry would be exposed as > > true to upper layers after your change, which would apparently break > > HistoryBrowserTest.RedirectHistory because the refresh delay there is 0 (<=1). > > > > If you don't plan to change the behavior of did_replace_entry despite > migrating > > to NEW_PAGE, I'd need to continue working on this bug. Thanks. > > Further investigation suggests this bug is not on my way and actually meta > refresh tags are propagated well via page transition type modifiers. So I could > be off track with this change, although it does seem confusing to have two > subtly different semantics for did_replace_entry. Ok. I'm definitely hoping to set did_replace_entry to true for client redirects, since it seems broken to set it to false. And that's where I currently am with my draft CL to fix https://crbug.com/596707: https://chromium-review.googlesource.com/c/567683. (I'm reclassifying location.replace and client redirects as NEW_PAGE with did_replace_entry, and I'm removing the line we're discussing here.) Unfortunately, that means I also have to figure out what's going on in the history service, and how to get that test to pass. That's a bit beyond what I can get done before leaving on vacation tomorrow, so it probably won't happen before branch cut. Glad to hear that it doesn't seem to be blocking you.
On 2017/07/12 20:14:15, Charlie Reis (slow) wrote: > On 2017/07/12 13:30:15, mastiz wrote: > > On 2017/07/12 05:15:40, mastiz wrote: > > > On 2017/07/11 22:07:05, Charlie Reis (slow) wrote: > > > > On 2017/07/11 19:08:59, mastiz wrote: > > > > > On 2017/07/10 18:53:17, mastiz wrote: > > > > > > Thanks for the detailed reply. > > > > > > > > > > > > On 2017/07/07 22:52:55, Charlie Reis (slow) wrote: > > > > > > > On 2017/07/06 15:03:19, jam wrote: > > > > > > > > On 2017/07/05 16:07:48, Avi (ping after 24h) wrote: > > > > > > > > > On 2017/07/05 13:16:35, mastiz wrote: > > > > > > > > > > On 2017/07/05 11:23:08, mastiz wrote: > > > > > > > > > > > avi@: Does a change of this kind make sense to you? I came > > > across > > > > > this > > > > > > > > > > weirdness > > > > > > > > > > > when debugging an issue with favicons (linked in the patch). > > It > > > > > > doesn't > > > > > > > > > solve > > > > > > > > > > > the whole problem but it seems a step forward. I'm also not > > > aware > > > > of > > > > > > all > > > > > > > > the > > > > > > > > > > > side effects of this change. > > > > > > > > > > > > > > > > > > > > There seem to be a few tests specifically verifying such > > behavior: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/chrome/browser/history/history_browserte... > > > > > > > > > > > > > > > > > > > > I'd be interested in knowing the rationale behind this logic. > > > > > > > > > > > > > > > > > > +jam who added this test in > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromiumcodereview.appspot.com/10915002?_ga=2.248047175.1149932859.14..., > > > > > > > > > although it was a port of a pyauto test, he might know > > > > > > > > > > > > > > > > Sorry, not really knowledgable in this area. I defer to Charlie. > > > > > > > > > > > > > > > > > +charlie who is also into nav > > > > > > > > > > > > > > > > > > I'm not personally attached to the behavior of meta > > > > http-equiv="refresh" > > > > > > > bring > > > > > > > > > treated as a new page rather than a redirect but since we've had > > it > > > so > > > > > > long > > > > > > > > this > > > > > > > > > way, changing it would be web-visible. Does a relevant spec have > > any > > > > > > > guidance > > > > > > > > > here? How do other browsers treat it? Changing the behavior may > > well > > > > > > require > > > > > > > > an > > > > > > > > > Intent. :( > > > > > > > > > > > > > > This is really interesting. At first, it looks like a clear > > regression > > > > > added > > > > > > in > > > > > > > Avi's CL, where is_same_document should make us set > did_replace_entry > > to > > > > > true > > > > > > > but never force it to false. But if that's the case, why is an even > > > older > > > > > > test > > > > > > > failing with the change? (How would it have passed before Avi's > CL?) > > > > > > > > > > > > I think the > > > > > > > > > > > > > > > > > > > > Maybe it has something to do with how we used to pull > > did_replace_entry > > > > from > > > > > > the > > > > > > > pending_entry_ (at the time Avi's CL landed) but now we let the > > renderer > > > > > tell > > > > > > us > > > > > > > (via should_replace_current_entry)? Maybe that change would have > > caused > > > > the > > > > > > > test to fail if not for this other bug? > > > > > > > > > > > > > > I'll point out that we don't seem to leave a NavigationEntry around > > for > > > > > > > redirector.html with or without this CL. The change is only visible > > to > > > > > > > observers of the navigation, like the Chrome history database in the > > > test. > > > > > > > > > So > > > > > > > maybe it's not such a disruptive change, and we could even update > the > > > > > history > > > > > > > code if needed. > > > > > > > > > > > > > > Still, I'm having trouble understanding the code itself (as noted > > > below), > > > > > and > > > > > > > why is_same_document implies did_replace_entry for EXISTING_PAGE > > > > > navigations. > > > > > > > In fact, it's a bit weird to me that this is considered > EXISTING_PAGE > > in > > > > the > > > > > > > first place-- it feels kind of like https://crbug.com/596707, where > it > > > > > should > > > > > > be > > > > > > > classified as NEW_PAGE with replacement rather than EXISTING_PAGE. > I > > > > wonder > > > > > > if > > > > > > > > > > Would the above involve both did_create_new_entry==true and > > > > > should_replace_current_entry? Is that not a contradiction? > > > > > > > > > > In general, I realized the implications of https://crbug.com/596707 are > > more > > > > > complex than I first thought. creis@, is this something you plan to > > address? > > > > > > > > > > Thanks, > > > > > Mikel > > > > > > > > It's not a contradiction-- we can and should create a new NavigationEntry > > > (i.e., > > > > did_create_new_entry) which replaces the current one (i.e., > > > > should_replace_current_entry). We should be doing that for > location.replace > > > and > > > > probably this meta refresh case as well, since these navigations can be > > > > cross-site and cross-process, and we shouldn't just try to update the > > existing > > > > NavigationEntry (as we would for something like history.replaceState). > > > > > > > > Yes, this is an involved change, and I've been putting it off. I can try > to > > > > spend some time on it, though. > > > > > > > > Can you help me understand one aspect of this? How did this bug relate to > > the > > > > favicon issue in https://crbug.com/498436 Did details->did_replace_entry > == > > > > false affect something in the favicon code? (Just trying to close the > loop > > to > > > > understand why it's required, and to make sure we can write tests as > > > > appropriate.) > > > > > > https://crbug.com/498436 doesn't affect favicon logic (AFAIK) as per today. > > I'm > > > trying to improve favicon logic > (https://codereview.chromium.org/2949023002/) > > to > > > better handle client-side redirects when I realized it was hard to tell some > > > cases apart (in particular meta refresh tags to different pages, because > > > did_replace_entry==false). I'm assuming did_replace_entry would be exposed > as > > > true to upper layers after your change, which would apparently break > > > HistoryBrowserTest.RedirectHistory because the refresh delay there is 0 > (<=1). > > > > > > If you don't plan to change the behavior of did_replace_entry despite > > migrating > > > to NEW_PAGE, I'd need to continue working on this bug. Thanks. > > > > Further investigation suggests this bug is not on my way and actually meta > > refresh tags are propagated well via page transition type modifiers. So I > could > > be off track with this change, although it does seem confusing to have two > > subtly different semantics for did_replace_entry. > > Ok. I'm definitely hoping to set did_replace_entry to true for client > redirects, since it seems broken to set it to false. And that's where I > currently am with my draft CL to fix https://crbug.com/596707: > https://chromium-review.googlesource.com/c/567683. (I'm reclassifying > location.replace and client redirects as NEW_PAGE with did_replace_entry, and > I'm removing the line we're discussing here.) > > Unfortunately, that means I also have to figure out what's going on in the > history service, and how to get that test to pass. That's a bit beyond what I > can get done before leaving on vacation tomorrow, so it probably won't happen > before branch cut. Glad to hear that it doesn't seem to be blocking you. Thanks! I'm closing this bug because I could fix my problem in https://chromium-review.googlesource.com/569760. Whether meta refresh tags should replace previous entries in history is an orthogonal discussion and one I don't have cycles for right now, but my personal impression is that it should be changed. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
