Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2696683002:
isShowingErrorPage should have really useful info (Closed)
DescriptionisShowingErrorPage should have really useful info
currently: is not updated for example from didFinishNavigation in TabWebContentObserver.java (if errorCode!=0...)
with patch: is updated in situation when error happen
BUG=692810
Patch Set 1 #
Total comments: 4
Messages
Total messages: 19 (7 generated)
Description was changed from ========== isShowingErrorPage should have really useful info BUG= ========== to ========== isShowingErrorPage should have really useful info currently: is not always updated is should be with patch: is setup in situation when error happen ==========
marcin@mwiacek.com changed reviewers: + bauerb@chromium.org
Description was changed from ========== isShowingErrorPage should have really useful info currently: is not always updated is should be with patch: is setup in situation when error happen ========== to ========== isShowingErrorPage should have really useful info currently: is not always updated when should be with patch: is setup in situation when error happen ==========
Can you explain this patch in a bit more detail, ideally in a bug? In what situation is the flag not updated, and how does that manifest to the user? See also https://chris.beams.io/posts/git-commit/ for the CL description. https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1528: setIsShowingErrorPage(showingErrorPage); I think you could just update |mIsShowingErrorPage| directly.
Description was changed from ========== isShowingErrorPage should have really useful info currently: is not always updated when should be with patch: is setup in situation when error happen ========== to ========== isShowingErrorPage should have really useful info currently: is not always updated when should be with patch: is setup in situation when error happen BUG=692810 ==========
Description was changed from ========== isShowingErrorPage should have really useful info currently: is not always updated when should be with patch: is setup in situation when error happen BUG=692810 ========== to ========== isShowingErrorPage should have really useful info currently: is not updated for example from didFinishNavigation in TabWebContentObserver.java (if errorCode!=0...) with patch: is updated in situation when error happen BUG=692810 ==========
> Can you explain this patch in a bit more detail, ideally in a bug? In what > situation is the flag not updated, and how does that manifest to the user? See > also https://chris.beams.io/posts/git-commit/ for the CL description. I've opened bug, user probably don't see probably problem, but... it's visible when you try to use variable and see false value for real error pages
https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1528: setIsShowingErrorPage(showingErrorPage); On 2017/02/13 22:51:30, Bernhard Bauer wrote: > I think you could just update |mIsShowingErrorPage| directly. I wanted to use method
bauerb@chromium.org changed reviewers: + shaktisahu@chromium.org
+shaktisahu I still don't quite understand the rationale behind this change. Changing the value of the |mIsShowingErrorPage| flag is going to affect call sites that read it -- have you verified that this doesn't introduce any issues? For that matter, the value of showingErrorPage is coming from the NavigationHandle in web_contents_observer_proxy.cc, but I don't really know what that means when we're _starting_ a navigation. Shakti, do you know more? https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1528: setIsShowingErrorPage(showingErrorPage); On 2017/02/15 23:40:41, marcin wrote: > On 2017/02/13 22:51:30, Bernhard Bauer wrote: > > I think you could just update |mIsShowingErrorPage| directly. > > I wanted to use method Yes, but why? I don't see any advantage in going through the public interface, in particular as the method has no additional logic beyond setting the member.
On 2017/02/13 22:51:30, Bernhard Bauer wrote: > Can you explain this patch in a bit more detail, ideally in a bug? In what > situation is the flag not updated, and how does that manifest to the user? See > also https://chris.beams.io/posts/git-commit/ for the CL description. > > https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): > > https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1528: > setIsShowingErrorPage(showingErrorPage); > I think you could just update |mIsShowingErrorPage| directly. marcin@ - Can you rebase this CL and verify if the bug exists? I think I have fixed this bug yesterday. https://codereview.chromium.org/2695843003/
See
if (errorCode != 0) {
mTab.updateThemeColorIfNeeded(true);
if (isInMainFrame) mTab.didFailPageLoad(errorCode);
recordErrorInPolicyAuditor(url, errorDescription, errorCode);
}
This is code from didFinishNavigation from
https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr...
Out variable is not updated for example then.
Name of variable is isShowingErrorPage
It should show reality. It’s updated for example in the same function in the
if (isInMainFrame) {
mTab.setIsTabStateDirty(true);
mTab.updateTitle();
mTab.handleDidFinishNavigation(url, pageTransition);
mTab.setIsShowingErrorPage(isErrorPage);
}
Currently it doesn’t handle various cases.
What’s the rationale of this change ? Less problems with debugging, when
somebody really need it.
With patch it’s set to true for example with Offline Page Error, which I believe
makes it useful.
Best Regards,
M.
From: bauerb@chromium.org [mailto:bauerb@chromium.org]
Sent: Thursday, February 16, 2017 13:29
To: marcin@mwiacek.com; shaktisahu@chromium.org
Cc: chromium-reviews@chromium.org; agrieve+watch@chromium.org
Subject: Re: isShowingErrorPage should have really useful info (issue 2696683002
by marcin@mwiacek.com)
+shaktisahu
I still don't quite understand the rationale behind this change. Changing the
value of the |mIsShowingErrorPage| flag is going to affect call sites that read
it -- have you verified that this doesn't introduce any issues? For that
matter,
the value of showingErrorPage is coming from the NavigationHandle in
web_contents_observer_proxy.cc, but I don't really know what that means when
we're _starting_ a navigation. Shakti, do you know more?
https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java
(right):
https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1528:
setIsShowingErrorPage(showingErrorPage);
On 2017/02/15 23:40:41, marcin wrote:
> On 2017/02/13 22:51:30, Bernhard Bauer wrote:
> > I think you could just update |mIsShowingErrorPage| directly.
>
> I wanted to use method
Yes, but why? I don't see any advantage in going through the public
interface, in particular as the method has no additional logic beyond
setting the member.
https://codereview.chromium.org/2696683002/
--
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.
> marcin@ - Can you rebase this CL and verify if the bug exists? I think I have > fixed this bug yesterday. > https://codereview.chromium.org/2695843003/ After short looking into code I see, that you setup this variable only when isInMainFrame is true. What in situation when it's false ? I'm doing it in both cases.
marcin@mwiacek.com changed reviewers: - marcin.wiacek@mwiacek.com
On 2017/02/16 18:27:48, marcin.wiacek_mwiacek.com wrote: > See > > > > if (errorCode != 0) { > > mTab.updateThemeColorIfNeeded(true); > > if (isInMainFrame) mTab.didFailPageLoad(errorCode); > > > > recordErrorInPolicyAuditor(url, errorDescription, errorCode); > > } > > > > This is code from didFinishNavigation from > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > Out variable is not updated for example then. > > Name of variable is isShowingErrorPage > > It should show reality. It’s updated for example in the same function in the > > if (isInMainFrame) { > mTab.setIsTabStateDirty(true); > mTab.updateTitle(); > mTab.handleDidFinishNavigation(url, pageTransition); > mTab.setIsShowingErrorPage(isErrorPage); > } > > > Currently it doesn’t handle various cases. > > What’s the rationale of this change ? Less problems with debugging, when > somebody really need it. > > With patch it’s set to true for example with Offline Page Error, which I believe > makes it useful. > > Best Regards, > M. > > > > From: mailto:bauerb@chromium.org [mailto:bauerb@chromium.org] > Sent: Thursday, February 16, 2017 13:29 > To: mailto:marcin@mwiacek.com; mailto:shaktisahu@chromium.org > Cc: mailto:chromium-reviews@chromium.org; mailto:agrieve+watch@chromium.org > Subject: Re: isShowingErrorPage should have really useful info (issue 2696683002 > by mailto:marcin@mwiacek.com) > > > > +shaktisahu > > I still don't quite understand the rationale behind this change. Changing the > value of the |mIsShowingErrorPage| flag is going to affect call sites that read > it -- have you verified that this doesn't introduce any issues? For that > matter, > the value of showingErrorPage is coming from the NavigationHandle in > web_contents_observer_proxy.cc, but I don't really know what that means when > we're _starting_ a navigation. Shakti, do you know more? > > > https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... > File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java > (right): > > https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... > chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1528: > setIsShowingErrorPage(showingErrorPage); > On 2017/02/15 23:40:41, marcin wrote: > > On 2017/02/13 22:51:30, Bernhard Bauer wrote: > > > I think you could just update |mIsShowingErrorPage| directly. > > > > I wanted to use method > > Yes, but why? I don't see any advantage in going through the public > interface, in particular as the method has no additional logic beyond > setting the member. > > https://codereview.chromium.org/2696683002/ > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. Have you verified this? May be you should run a dry run and see if it breaks something.
https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right): https://codereview.chromium.org/2696683002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1566: setIsShowingErrorPage(false); Why is this false? It should reflect the same value as |isErrorPage| as received from NavigationHandle in TabWebContentsObserver. I believe |isErrorPage| is the absolute truth about the state of the navigation.
On 2017/02/16 18:48:56, marcin wrote: > > marcin@ - Can you rebase this CL and verify if the bug exists? I think I have > > fixed this bug yesterday. > > https://codereview.chromium.org/2695843003/ > > After short looking into code I see, that you setup this variable only when > isInMainFrame is true. > > What in situation when it's false ? > > I'm doing it in both cases. We shouldn't be showing an error page if the error happens only in a subframe and main frame should still show the contents.
On 2017/02/16 18:57:24, shaktisahu wrote: > On 2017/02/16 18:48:56, marcin wrote: > > > marcin@ - Can you rebase this CL and verify if the bug exists? I think I > have > > > fixed this bug yesterday. > > > https://codereview.chromium.org/2695843003/ > > > > After short looking into code I see, that you setup this variable only when > > isInMainFrame is true. > > > > What in situation when it's false ? > > > > I'm doing it in both cases. > > We shouldn't be showing an error page if the error happens only in a subframe > and main frame should still show the contents. OK, I think, that I can close my patch now. thx |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
