Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2397453002:
Asynchronously resolve getAvailability callbacks. (Closed)
|
Created:
4 years, 2 months ago by mlamouri (slow - plz ping) Modified:
4 years, 2 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAsynchronously resolve getAvailability callbacks.
BUG=651216
R=mfoltz@chromium.org
Committed: https://crrev.com/d984d26caeefead1e3268d562955d6e560fbcd39
Cr-Commit-Position: refs/heads/master@{#423553}
Patch Set 1 #Patch Set 2 : add tests #
Total comments: 4
Patch Set 3 : fixes #Patch Set 4 : fix compile #
Messages
Total messages: 35 (19 generated)
The CQ bit was checked by mlamouri@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...
Mark, I couldn't find the root cause of this. There is clearly something happening because we delete the callback in fly but it's really unclear to me why. I even wrote a layout test that, as far as I can tell is exactly similar to what I ran on content_shell devtools but I couldn't reproduce the bug there. I wouldn't be surprised if that it is some odd GC issue. I think the fix works and is small enough to attempt to send to M54.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I tried another approach and I was able to get a content_browsertest to reproduce the issue.
LGTM with minor comments. https://codereview.chromium.org/2397453002/diff/20001/content/browser/present... File content/browser/presentation/presentation_browsertest.cc (right): https://codereview.chromium.org/2397453002/diff/20001/content/browser/present... content/browser/presentation/presentation_browsertest.cc:25: "var r = new PresentationRequest('foo.html')"); nit: s/var/const/ here and below (assuming it doesn't change the meaning of the test) https://codereview.chromium.org/2397453002/diff/20001/content/browser/present... content/browser/presentation/presentation_browsertest.cc:35: "p1 === undefined")->GetAsBoolean(&is_p1_undefined); There's a spec change coming that getAvailability() will return the same object in the same script context. Can you add a TODO(crbug.com/653131) here to add an assertion that p1 === p2?
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org Link to the patchset: https://codereview.chromium.org/2397453002/#ps40001 (title: "fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2397453002/diff/20001/content/browser/present... File content/browser/presentation/presentation_browsertest.cc (right): https://codereview.chromium.org/2397453002/diff/20001/content/browser/present... content/browser/presentation/presentation_browsertest.cc:25: "var r = new PresentationRequest('foo.html')"); On 2016/10/05 at 16:04:01, mark a. foltz wrote: > nit: s/var/const/ here and below (assuming it doesn't change the meaning of the test) Sure. https://codereview.chromium.org/2397453002/diff/20001/content/browser/present... content/browser/presentation/presentation_browsertest.cc:35: "p1 === undefined")->GetAsBoolean(&is_p1_undefined); On 2016/10/05 at 16:04:01, mark a. foltz wrote: > There's a spec change coming that getAvailability() will return the same object in the same script context. > Can you add a TODO(crbug.com/653131) here to add an assertion that p1 === p2? Done. Though, I would expect us to test this in Blink.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
mlamouri@chromium.org changed reviewers: + thakis@chromium.org
+thakis@ for the BUILD file change.
build.gn lgtm
The CQ bit was checked by mlamouri@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Description was changed from ========== Asynchronously resolve getAvailability callbacks. BUG=651216 R=mfoltz@chromium.org ========== to ========== Asynchronously resolve getAvailability callbacks. BUG=651216 R=mfoltz@chromium.org TBR=jochen@chromium.org (thakis@ review didn't approve BUILD.gn change) ==========
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org, thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2397453002/#ps60001 (title: "fix compile")
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 mlamouri@chromium.org
Description was changed from ========== Asynchronously resolve getAvailability callbacks. BUG=651216 R=mfoltz@chromium.org TBR=jochen@chromium.org (thakis@ review didn't approve BUILD.gn change) ========== to ========== Asynchronously resolve getAvailability callbacks. BUG=651216 R=mfoltz@chromium.org ==========
mlamouri@chromium.org changed reviewers: + jochen@chromium.org
phajdan.jr@chromium.org changed reviewers: + phajdan.jr@chromium.org
LGTM
The CQ bit was checked by mlamouri@chromium.org
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 #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Asynchronously resolve getAvailability callbacks. BUG=651216 R=mfoltz@chromium.org ========== to ========== Asynchronously resolve getAvailability callbacks. BUG=651216 R=mfoltz@chromium.org Committed: https://crrev.com/d984d26caeefead1e3268d562955d6e560fbcd39 Cr-Commit-Position: refs/heads/master@{#423553} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/d984d26caeefead1e3268d562955d6e560fbcd39 Cr-Commit-Position: refs/heads/master@{#423553} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
