Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2939503002:
[DevTools] Clear console on GlobalObjectCleared (Closed)
|
Created:
3 years, 6 months ago by dgozman Modified:
3 years, 6 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Description[DevTools] Clear console on GlobalObjectCleared
Previously we did it on MainFrameStaredLoading instead, but that is too early.
BUG=714292
Review-Url: https://codereview.chromium.org/2939503002
Cr-Commit-Position: refs/heads/master@{#479520}
Committed: https://chromium.googlesource.com/chromium/src/+/1ba21006d3c29454dc12c3b6c3b011137d9159f4
Patch Set 1 #
Messages
Total messages: 22 (11 generated)
The CQ bit was checked by dgozman@chromium.org to run a CQ dry run
dgozman@chromium.org changed reviewers: + pfeldman@chromium.org
Could you please take a look?
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: This issue passed the CQ dry run.
kozyatinskiy@chromium.org changed reviewers: + kozyatinskiy@chromium.org
We changed it to frameStartLoading intentionally: https://codereview.chromium.org/2563883005 To support following warning message about cookies: https://codereview.chromium.org/2563083002/
On 2017/06/12 22:28:52, kozy wrote: > We changed it to frameStartLoading intentionally: > https://codereview.chromium.org/2563883005 > > To support following warning message about cookies: > https://codereview.chromium.org/2563083002/ Thanks! The test about cookies still passes though. There is a balance to be hold here, with GlobalObjectCleared we miss main request activity, but that's about it. I tested <img src='foo'> and these errors happen after. WDYT?
On 2017/06/12 22:34:28, dgozman wrote: > On 2017/06/12 22:28:52, kozy wrote: > > We changed it to frameStartLoading intentionally: > > https://codereview.chromium.org/2563883005 > > > > To support following warning message about cookies: > > https://codereview.chromium.org/2563083002/ > > Thanks! The test about cookies still passes though. I think if run this test manually a lot of times in some cases this message will be cleared. Or maybe something was changed. In general if most of the time we get cookie warning - I'm fine with this CL. > There is a balance to be hold here, with GlobalObjectCleared we miss main > request activity, but that's about it. I tested <img src='foo'> and these errors > happen after. WDYT? I'm wondering do messages before and after reload have the same execution context id and do we receive executionContextDestroyed on navigation? Can we hide messages for destroyed contexts?
On 2017/06/13 09:22:15, kozy wrote: > On 2017/06/12 22:34:28, dgozman wrote: > > On 2017/06/12 22:28:52, kozy wrote: > > > We changed it to frameStartLoading intentionally: > > > https://codereview.chromium.org/2563883005 > > > > > > To support following warning message about cookies: > > > https://codereview.chromium.org/2563083002/ > > > > Thanks! The test about cookies still passes though. > > I think if run this test manually a lot of times in some cases this message will > be cleared. Or maybe something was changed. In general if most of the time we > get cookie warning - I'm fine with this CL. > > > There is a balance to be hold here, with GlobalObjectCleared we miss main > > request activity, but that's about it. I tested <img src='foo'> and these > errors > > happen after. WDYT? > > I'm wondering do messages before and after reload have the same execution > context id and do we receive executionContextDestroyed on navigation? Can we > hide messages for destroyed contexts? Well, GlobalObjectCleared is exactly that - it's fired upon ExecutionContextsCleared from Runtime.
On 2017/06/13 17:23:57, dgozman wrote: > On 2017/06/13 09:22:15, kozy wrote: > > On 2017/06/12 22:34:28, dgozman wrote: > > > On 2017/06/12 22:28:52, kozy wrote: > > > > We changed it to frameStartLoading intentionally: > > > > https://codereview.chromium.org/2563883005 > > > > > > > > To support following warning message about cookies: > > > > https://codereview.chromium.org/2563083002/ > > > > > > Thanks! The test about cookies still passes though. > > > > I think if run this test manually a lot of times in some cases this message > will > > be cleared. Or maybe something was changed. In general if most of the time we > > get cookie warning - I'm fine with this CL. > > > > > There is a balance to be hold here, with GlobalObjectCleared we miss main > > > request activity, but that's about it. I tested <img src='foo'> and these > > errors > > > happen after. WDYT? > > > > I'm wondering do messages before and after reload have the same execution > > context id and do we receive executionContextDestroyed on navigation? Can we > > hide messages for destroyed contexts? > > Well, GlobalObjectCleared is exactly that - it's fired upon > ExecutionContextsCleared from Runtime. lgtm. My point here is to clear not all console message on navigation but only console messages from destroyed execution context.
> My point here is to clear not all console message on navigation but only console > messages from destroyed execution context. That's an interesting idea, but we lack execution context information for some messages. Also, we don't want to remove messages when execution context is gone every time, only on navigation. Otherwise, you'll miss all worker messages for example.
The CQ bit was checked by dgozman@chromium.org
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
Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by dgozman@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1497474733082660, "parent_rev":
"bd608208e501cb0572d4466e3e372c02b64c564c", "commit_rev":
"1ba21006d3c29454dc12c3b6c3b011137d9159f4"}
Message was sent while issue was closed.
Description was changed from ========== [DevTools] Clear console on GlobalObjectCleared Previously we did it on MainFrameStaredLoading instead, but that is too early. BUG=714292 ========== to ========== [DevTools] Clear console on GlobalObjectCleared Previously we did it on MainFrameStaredLoading instead, but that is too early. BUG=714292 Review-Url: https://codereview.chromium.org/2939503002 Cr-Commit-Position: refs/heads/master@{#479520} Committed: https://chromium.googlesource.com/chromium/src/+/1ba21006d3c29454dc12c3b6c3b0... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/1ba21006d3c29454dc12c3b6c3b0... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
