Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2449393002:
Fix losing selected language on ToS page reloading. (Closed)
DescriptionFix losing selected language on ToS page reloading.
The reloading implemented by setting url to the
ToS page directly. However, it has some redirection and
user may select another language.
Setting the page again triggers to redirection to the
default page. Also, it sometimes triggers another redirection
which causes flickering of the page.
Fix the problems by reload().
BUG=659633
TEST=Ran on device. Ran trybots.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/03d3d5a0d122ab8ab1289cbca2af84377c924415
Cr-Commit-Position: refs/heads/master@{#429796}
Patch Set 1 #
Total comments: 5
Patch Set 2 : rebase #Messages
Total messages: 27 (10 generated)
Description was changed from ========== Fix losing selected language on ToS page reloading. The reloading implemented by setting url to the ToS page directly. However, it has some redirection and user may select another language. Setting the page again triggers to redirection to the default page. Also, it sometimes triggers another redirection which causes flickering of the page. Fix the problems by reload(). BUG=659633 TEST=Ran on device. Ran trybots. ========== to ========== Fix losing selected language on ToS page reloading. The reloading implemented by setting url to the ToS page directly. However, it has some redirection and user may select another language. Setting the page again triggers to redirection to the default page. Also, it sometimes triggers another redirection which causes flickering of the page. Fix the problems by reload(). BUG=659633 TEST=Ran on device. Ran trybots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by hidehiko@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...
hidehiko@chromium.org changed reviewers: + xiyuan@chromium.org
PTAL: Xiyuan. FYI: Luis, Yury. Note: my pending refactoring CL can be rebased on top of this CL.
https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/arc_support/background.js:674: loadTerms(); Note: Typically, this is the reload case. However, the error page can be shown "before" the ToS page, so this could be first loading, theoretically... That's why we share the loadTerms().
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
khmel@chromium.org changed reviewers: + khmel@chromium.org
https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/arc_support/background.js:505: if (termsView.src) { This might be dangerous. Assume ToS decides to redirect to some page, which is different from what we set during the page processing and user selection. This can lead to situation when ToS gets stuck on this page. That is why full reload was implemented which is safer. Could you please make sure that src URL looks at least as ToS target.
Thank you for review. https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/arc_support/background.js:505: if (termsView.src) { On 2016/10/26 20:35:20, khmel wrote: > This might be dangerous. Assume ToS decides to redirect to some page, which is > different from what we set during the page processing and user selection. This > can lead to situation when ToS gets stuck on this page. That is why full reload > was implemented which is safer. Could you please make sure that src URL looks at > least as ToS target. I don't think the old code handles the case, because there is no explicit way for a user to show RETRY button intentionally. Practically, the easiest way, I think, is closing the Chrome App and re-enable ARC, so that the Chrome App is shown again. The behavior is still preserved, even with this CL. Also, if ToS page navigates a user to another page, it should be considered still as a part of ToS? I think it's ToS's responsibility, shouldn't it?
https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/arc_support/background.js:505: if (termsView.src) { On 2016/10/27 04:01:48, hidehiko wrote: > On 2016/10/26 20:35:20, khmel wrote: > > This might be dangerous. Assume ToS decides to redirect to some page, which is > > different from what we set during the page processing and user selection. This > > can lead to situation when ToS gets stuck on this page. That is why full > reload > > was implemented which is safer. Could you please make sure that src URL looks > at > > least as ToS target. > > I don't think the old code handles the case, because there is no explicit way > for a user to show RETRY button intentionally. Practically, the easiest way, I > think, is closing the Chrome App and re-enable ARC, so that the Chrome App is > shown again. The behavior is still preserved, even with this CL. Sorry, don't get what do you mean? Old code has simple logic. Navigate to default target, select option based on current combination of lang and location and load terms for it. In case of error it repeats this step. > > Also, if ToS page navigates a user to another page, it should be considered > still as a part of ToS? I think it's ToS's responsibility, shouldn't it? It is potentially possible to change several pages when coming to final user selection. My concern is before reloading this particular page let make sure that it looks like https://play.google.com/intl/.../about/play-terms.html. Otherwise you may continue reloading wrong page, for example some https://google.com/not_allowed, wrong_dns or any other not related to ToS
https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... File chrome/browser/resources/chromeos/arc_support/background.js (right): https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... chrome/browser/resources/chromeos/arc_support/background.js:505: if (termsView.src) { On 2016/10/27 14:40:57, khmel wrote: > On 2016/10/27 04:01:48, hidehiko wrote: > > On 2016/10/26 20:35:20, khmel wrote: > > > This might be dangerous. Assume ToS decides to redirect to some page, which > is > > > different from what we set during the page processing and user selection. > This > > > can lead to situation when ToS gets stuck on this page. That is why full > > reload > > > was implemented which is safer. Could you please make sure that src URL > looks > > at > > > least as ToS target. > > > > I don't think the old code handles the case, because there is no explicit way > > for a user to show RETRY button intentionally. Practically, the easiest way, I > > think, is closing the Chrome App and re-enable ARC, so that the Chrome App is > > shown again. The behavior is still preserved, even with this CL. > Sorry, don't get what do you mean? Old code has simple logic. Navigate to > default target, select option based on current combination of lang and location > and load terms for it. In case of error it repeats this step. Sorry, I still am confused. "ToS decides to redirect to some page, which is different from what we set during the page processing and user selection." So the user see the redirected webpage, on ToS page. So, the user won't see the RETRY button??? > > > > > Also, if ToS page navigates a user to another page, it should be considered > > still as a part of ToS? I think it's ToS's responsibility, shouldn't it? > > It is potentially possible to change several pages when coming to final user > selection. My concern is before reloading this particular page let make sure > that it looks like https://play.google.com/intl/.../about/play-terms.html. > Otherwise you may continue reloading wrong page, for example some > https://google.com/not_allowed, wrong_dns or any other not related to ToS > I also didn't get understand your point yet... How users are navigated to such a page? If it is linked from ToS webpage, then should we think it is a part of ToS? Or, if what you said is "We should not show the page other than play.google.com/intl/${lang}/about/play-terms.html in any cases", then this function is not the place to handle it, IIUC, considering redirection.
On 2016/10/27 16:41:01, hidehiko wrote: > https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... > File chrome/browser/resources/chromeos/arc_support/background.js (right): > > https://codereview.chromium.org/2449393002/diff/1/chrome/browser/resources/ch... > chrome/browser/resources/chromeos/arc_support/background.js:505: if > (termsView.src) { > On 2016/10/27 14:40:57, khmel wrote: > > On 2016/10/27 04:01:48, hidehiko wrote: > > > On 2016/10/26 20:35:20, khmel wrote: > > > > This might be dangerous. Assume ToS decides to redirect to some page, > which > > is > > > > different from what we set during the page processing and user selection. > > This > > > > can lead to situation when ToS gets stuck on this page. That is why full > > > reload > > > > was implemented which is safer. Could you please make sure that src URL > > looks > > > at > > > > least as ToS target. > > > > > > I don't think the old code handles the case, because there is no explicit > way > > > for a user to show RETRY button intentionally. Practically, the easiest way, > I > > > think, is closing the Chrome App and re-enable ARC, so that the Chrome App > is > > > shown again. The behavior is still preserved, even with this CL. > > Sorry, don't get what do you mean? Old code has simple logic. Navigate to > > default target, select option based on current combination of lang and > location > > and load terms for it. In case of error it repeats this step. > > Sorry, I still am confused. > > "ToS decides to redirect to some page, which is > different from what we set during the page processing and user selection." > > So the user see the redirected webpage, on ToS page. So, the user won't see the > RETRY button??? > > > > > > > > > Also, if ToS page navigates a user to another page, it should be considered > > > still as a part of ToS? I think it's ToS's responsibility, shouldn't it? > > > > It is potentially possible to change several pages when coming to final user > > selection. My concern is before reloading this particular page let make sure > > that it looks like https://play.google.com/intl/.../about/play-terms.html. > > Otherwise you may continue reloading wrong page, for example some > > https://google.com/not_allowed, wrong_dns or any other not related to ToS > > > > I also didn't get understand your point yet... > > How users are navigated to such a page? > If it is linked from ToS webpage, then should we think it is a part of ToS? > > Or, if what you said is "We should not show the page other than > play.google.com/intl/${lang}/about/play-terms.html in any cases", then this > function is not the place to handle it, IIUC, considering redirection. I have only concern (you may consider this as nit): Before reloading ToS let make sure that web.src 1. starts with 'https://play.google.com/intl/' 2. ends with '/about/play-terms.html' to be double sure that we are reloading what is expected. FYI: ${lang} is actually not a lang, it is combination of lang and location in common case and in some exceptions this is just lang. Again, consider this as nit. Feel free to ignore this if you think this would never happen. lgtm
On 2016/10/27 16:48:05, khmel wrote:
> I have only concern (you may consider this as nit): Before reloading ToS let
> make sure that web.src
> 1. starts with 'https://play.google.com/intl/'
> 2. ends with '/about/play-terms.html'
> to be double sure that we are reloading what is expected.
>
> FYI: ${lang} is actually not a lang, it is combination of lang and location in
> common case and in some exceptions this is just lang.
>
> Again, consider this as nit. Feel free to ignore this if you think this would
> never happen.
>
> lgtm
Think Yury has a point here. That just doing .reload is not exactly what we
want. Another example is that user gets to the terms page when the device is
connected to a portal network. Thus we might be reloading the portal page. Most
of the portal page does not auto redirect back to where user comes from.
Checking the termsView.src would make sense in such scenario.
On 2016/10/27 16:52:00, xiyuan wrote:
> On 2016/10/27 16:48:05, khmel wrote:
> > I have only concern (you may consider this as nit): Before reloading ToS let
> > make sure that web.src
> > 1. starts with 'https://play.google.com/intl/'
> > 2. ends with '/about/play-terms.html'
> > to be double sure that we are reloading what is expected.
Because I still does not get your point, it is difficult for me to say
it is important or not, TBH.
I understand what you want me to do, but I do not understand "for what" in
details, yet.
If you list several scenarios you're concerning about, it will help me
understand your point a lot.
> >
> > FYI: ${lang} is actually not a lang, it is combination of lang and location
in
> > common case and in some exceptions this is just lang.
> >
Ah, so I should have said "locale" ;-).
> > Again, consider this as nit. Feel free to ignore this if you think this
would
> > never happen.
> >
> > lgtm
>
> Think Yury has a point here. That just doing .reload is not exactly what we
> want. Another example is that user gets to the terms page when the device is
> connected to a portal network. Thus we might be reloading the portal page.
Most
> of the portal page does not auto redirect back to where user comes from.
> Checking the termsView.src would make sense in such scenario.
Portal page case cannot be rescued, unfortunately, IIUC,
because the error page is not shown in such a case, so that the user cannot
trigger RETRY.
IIUC, to reload ToS page, what user can do is close the window and reopen by
re-enabling ARC.
The re-enabling ARC loads the "https://play.google.com/about/play-terms.html" as
it is in initial case,
and that behavior is not changed in this CL.
BTW, what is the expected behavior for portal page case?
I think it is necessary to be addressed separately, if it is not yet well
discussed.
WDYT?
On 2016/10/28 06:24:52, hidehiko wrote: > Portal page case cannot be rescued, unfortunately, IIUC, > because the error page is not shown in such a case, so that the user cannot > trigger RETRY. > IIUC, to reload ToS page, what user can do is close the window and reopen by > re-enabling ARC. > The re-enabling ARC loads the "https://play.google.com/about/play-terms.html" as > it is in initial case, > and that behavior is not changed in this CL. > > BTW, what is the expected behavior for portal page case? > I think it is necessary to be addressed separately, if it is not yet well > discussed. > > WDYT? You are right. Portal page is a bad example since it would not trigger error page so not a problem here for "Retry". However, Yury's point is that we should make sure the termsView.src is something looks like ToS page before issuing .reload(). I think that is still a valid point. What if termsView hits a 502 error page while loading ToS page? This triggers the error page and show the retry button. And in this case, .reload is not right and we should set termsView.src instead.
On 2016/10/28 16:37:20, xiyuan wrote: > On 2016/10/28 06:24:52, hidehiko wrote: > > Portal page case cannot be rescued, unfortunately, IIUC, > > because the error page is not shown in such a case, so that the user cannot > > trigger RETRY. > > IIUC, to reload ToS page, what user can do is close the window and reopen by > > re-enabling ARC. > > The re-enabling ARC loads the "https://play.google.com/about/play-terms.html" > as > > it is in initial case, > > and that behavior is not changed in this CL. > > > > BTW, what is the expected behavior for portal page case? > > I think it is necessary to be addressed separately, if it is not yet well > > discussed. > > > > WDYT? > > You are right. Portal page is a bad example since it would not trigger error > page so not a problem here for "Retry". > > However, Yury's point is that we should make sure the termsView.src is something > looks like ToS page before issuing .reload(). I think that is still a valid > point. What if termsView hits a 502 error page while loading ToS page? This > triggers the error page and show the retry button. And in this case, .reload is > not right and we should set termsView.src instead. My question is; What is the ToS page in this context. Specifically, if some external page is being loaded, is it still a part of ToS? If so, should we leave the page as is? If not, should we prevent to show such a page in our ToS webview? Handling such a case only when reloading from the RETRY sounds a bit ad-hoc, to me. WDYT? BTW, webview.src in the function may not be updated: E.g. - Load ToS - Disconnect network to let loading fail. - Select another language and show error page. - Click RETRY. In this case, AFAI tested, webview.src is still old URL. I.e., webview.src = webview.src reloads the old page, but webview.reload() will reload the page which couldn't be loaded. I'm not sure whether this is spec or not, TBH. According to the doc: "Mirrors the logic in the browser's omnibox: either returning a pending new navigation if initiated by the embedder page, or the last committed navigation.", so it sounds like an edge case, anyway? So, checking webview.src here looks not a good approach to me (at least at the moment), and if we'd need/want to check here, then we should seek an alternative approach, I think? Thanks, - hidehiko
On 2016/11/01 17:04:24, hidehiko wrote: > In this case, AFAI tested, webview.src is still old URL. I.e., webview.src = > webview.src reloads the old page, but webview.reload() will reload the page > which couldn't be loaded. > > I'm not sure whether this is spec or not, TBH. According to the doc: > "Mirrors the logic in the browser's omnibox: either returning a pending new > navigation if initiated by the embedder page, or the last committed > navigation.", so it sounds like an edge case, anyway? > > So, checking webview.src here looks not a good approach to me (at least at the > moment), > and if we'd need/want to check here, then we should seek an alternative > approach, I think? Thank you for trying it out. It sounds like an unhandled edge case of webview. :( In this case, I agree that we should find an alternative to handle it in another CL. still lgtm
Thank you for review. Rebased and submitting.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xiyuan@chromium.org, khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2449393002/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix losing selected language on ToS page reloading. The reloading implemented by setting url to the ToS page directly. However, it has some redirection and user may select another language. Setting the page again triggers to redirection to the default page. Also, it sometimes triggers another redirection which causes flickering of the page. Fix the problems by reload(). BUG=659633 TEST=Ran on device. Ran trybots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Fix losing selected language on ToS page reloading. The reloading implemented by setting url to the ToS page directly. However, it has some redirection and user may select another language. Setting the page again triggers to redirection to the default page. Also, it sometimes triggers another redirection which causes flickering of the page. Fix the problems by reload(). BUG=659633 TEST=Ran on device. Ran trybots. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/03d3d5a0d122ab8ab1289cbca2af84377c924415 Cr-Commit-Position: refs/heads/master@{#429796} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/03d3d5a0d122ab8ab1289cbca2af84377c924415 Cr-Commit-Position: refs/heads/master@{#429796} |
