Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1160413004:
Add hooks for BeforeInstallPromptEvent to be redispatched. (Closed)
|
Created:
5 years, 6 months ago by dominickn (DO NOT USE) Modified:
5 years, 6 months ago Reviewers:
mlamouri (slow - plz ping) CC:
blink-reviews, chrome-apps-syd-reviews_chromium.org, dglazkov+blink Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionAdd hooks for BeforeInstallPromptEvent to be redispatched.
Sites may wish to prevent an app banner prompt display upon loading,
and defer the event until later in their own context. This CL provides
a prompt() method on the BeforeInstallPromptEvent, which can be called
to initiate this process. This completes the implementation of the event.
Intent to implement the beforeinstallprompt event:
https://groups.google.com/a/chromium.org/forum/#!msg/blink-dev/HSSqpbYd8W8/6I_InR9keNwJ
Intent to ship the event:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/OcJ0b7JPZ5M
Associated Chromium CL:
https://codereview.chromium.org/1173293006
BUG=490545
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=197461
Patch Set 1 #
Total comments: 4
Patch Set 2 : Tidying comments #Patch Set 3 : Adding prompt method to expected API #Patch Set 4 : Making prompt() return a promise #
Total comments: 4
Patch Set 5 : Returning Promise<void> and tidying implementation #
Total comments: 2
Patch Set 6 : Addressing reviewer comments #
Total comments: 1
Patch Set 7 : Renaming method per reviewer feedback #Patch Set 8 : #
Messages
Total messages: 41 (11 generated)
dominickn@google.com changed reviewers: + mlamouri@chromium.org
Please review this version of the patch (from my chromium.org account). Thanks!
https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... Source/modules/app_banner/BeforeInstallPromptEvent.cpp:68: // It will also silently fail if the event has already been redispatched. Why silently failing? Why not having prompt() return a Promise<> and reject the promise if the state isn't the expected one? It will also give leeway for UA to reject the prompt() request. https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... File Source/modules/app_banner/BeforeInstallPromptEvent.h (right): https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... Source/modules/app_banner/BeforeInstallPromptEvent.h:44: // wishes to redispatch the event at a later point in their own context. I don't think this comment is useful. prompt() is directly linked to the bindings. It's merely boilerplate at that point. https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: void prompt(); The tests are failing because of this addition. It changes the exposed interfaces to the Web which is checked by the following tests to prevent unexpected changes: webexposed/global-interface-listing.html virtual/stable/webexposed/global-interface-listing.html Note that virtual/stable/ tests the interface for a stable release while the first test tests for a "test" version. That also means that in order to land this change you will need to send an Intent to Implement and Ship in blink-dev and get it approved. Otherwise, you should send an "Intent to Implement", have the prompt() method implemented behind a flag and send an "Intent to Ship" when the method is ready to ship. More information can be found here: http://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process https://codereview.chromium.org/1160413004/diff/1/public/platform/modules/app... File public/platform/modules/app_banner/WebAppBannerClient.h (right): https://codereview.chromium.org/1160413004/diff/1/public/platform/modules/app... public/platform/modules/app_banner/WebAppBannerClient.h:29: // it has a dummy do-nothing implementation. nit: remove the comment. Not having the function pure virtual isn't a problem. It's fairly common for Blink interfaces. Alternatively, just leave a one-liner like "TODO(dominickn) make the method pure virtual when implemented by Chromium."
> https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: void prompt(); > The tests are failing because of this addition. It changes the exposed > interfaces to the Web which is checked by the following tests to prevent > unexpected changes: > webexposed/global-interface-listing.html > virtual/stable/webexposed/global-interface-listing.html > > Note that virtual/stable/ tests the interface for a stable release while the > first test tests for a "test" version. That also means that in order to land > this change you will need to send an Intent to Implement and Ship in blink-dev > and get it approved. Otherwise, you should send an "Intent to Implement", have > the prompt() method implemented behind a flag and send an "Intent to Ship" when > the method is ready to ship. > More information can be found here: > http://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process This is finishing up the implementation of the beforeinstallprompt event tracked at https://code.google.com/p/chromium/issues/detail?id=460945. There is already an Intent to Implement and Intent to Ship for the event itself, and the redispatching behaviour was described (with a slightly different API) in the original spec. Do I still need to provide a new set of these docs? > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): > > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > Source/modules/app_banner/BeforeInstallPromptEvent.cpp:68: // It will also > silently fail if the event has already been redispatched. > Why silently failing? Why not having prompt() return a Promise<> and reject the > promise if the state isn't the expected one? It will also give leeway for UA to > reject the prompt() request. I'll look into this. Thanks! > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > File Source/modules/app_banner/BeforeInstallPromptEvent.h (right): > > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > Source/modules/app_banner/BeforeInstallPromptEvent.h:44: // wishes to redispatch > the event at a later point in their own context. > I don't think this comment is useful. prompt() is directly linked to the > bindings. It's merely boilerplate at that point. Will do. > https://codereview.chromium.org/1160413004/diff/1/public/platform/modules/app... > File public/platform/modules/app_banner/WebAppBannerClient.h (right): > > https://codereview.chromium.org/1160413004/diff/1/public/platform/modules/app... > public/platform/modules/app_banner/WebAppBannerClient.h:29: // it has a dummy > do-nothing implementation. > nit: remove the comment. Not having the function pure virtual isn't a problem. > It's fairly common for Blink interfaces. > Alternatively, just leave a one-liner like "TODO(dominickn) make the method pure > virtual when implemented by Chromium." Will do.
On 2015/06/09 at 02:58:02, dominickn wrote: > > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > > Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: void prompt(); > > The tests are failing because of this addition. It changes the exposed > > interfaces to the Web which is checked by the following tests to prevent > > unexpected changes: > > webexposed/global-interface-listing.html > > virtual/stable/webexposed/global-interface-listing.html > > > > Note that virtual/stable/ tests the interface for a stable release while the > > first test tests for a "test" version. That also means that in order to land > > this change you will need to send an Intent to Implement and Ship in blink-dev > > and get it approved. Otherwise, you should send an "Intent to Implement", have > > the prompt() method implemented behind a flag and send an "Intent to Ship" when > > the method is ready to ship. > > More information can be found here: > > http://www.chromium.org/blink#TOC-Web-Platform-Changes:-Process > > This is finishing up the implementation of the beforeinstallprompt event tracked at https://code.google.com/p/chromium/issues/detail?id=460945. There is already an Intent to Implement and Intent to Ship for the event itself, and the redispatching behaviour was described (with a slightly different API) in the original spec. Do I still need to provide a new set of these docs? I think you should at least point to the original Intent to Ship that you are shipping a follow-up.
> I think you should at least point to the original Intent to Ship that you are > shipping a follow-up. I've added this to the CL description. Thanks for the feedback! > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > Source/modules/app_banner/BeforeInstallPromptEvent.cpp:68: // It will also > silently fail if the event has already been redispatched. > Why silently failing? Why not having prompt() return a Promise<> and reject the > promise if the state isn't the expected one? It will also give leeway for UA to > reject the prompt() request. The API proposal for the event (https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md) uses a Promise for the initial trigger, and a call to prompt() to restart the event pipeline after preventDefault(). It's intended that the redispatch can only happen in this context, and once redispatched you can access the userChoice promise on the event. This lets the event be handled consistently whether it's delayed or not. Is there extra utility in making prompt() also return a Promise? We'll need to go back and talk about the proposed API again if this is the case.
On 2015/06/09 at 08:03:29, dominickn wrote: > > I think you should at least point to the original Intent to Ship that you are > > shipping a follow-up. > > I've added this to the CL description. Thanks for the feedback! Thanks for updating the CL's description. Though, I might have not been very clear and I wanted you to send a message in the Intent to Ship thread to say that you are implementing this follow-up. I think having API OWNERS feedback would be a good thing here. > > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > > File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): > > > https://codereview.chromium.org/1160413004/diff/1/Source/modules/app_banner/B... > > Source/modules/app_banner/BeforeInstallPromptEvent.cpp:68: // It will also > > silently fail if the event has already been redispatched. > > Why silently failing? Why not having prompt() return a Promise<> and reject the > > promise if the state isn't the expected one? It will also give leeway for UA to > > reject the prompt() request. > > The API proposal for the event (https://github.com/slightlyoff/AppInstallImprovements/blob/master/explainer.md) uses a Promise for the initial trigger, and a call to prompt() to restart the event pipeline after preventDefault(). It's intended that the redispatch can only happen in this context, and once redispatched you can access the userChoice promise on the event. This lets the event be handled consistently whether it's delayed or not. Is there extra utility in making prompt() also return a Promise? We'll need to go back and talk about the proposed API again if this is the case. Having prompt() return a promise will let the developer know if the call actually succeeded. As is, the call will fail silently which is generally a bad thing. Furthermore, some UAs might want to reject the prompt request in some situations so it wouldn't be a bad thing to let this door open in the API even if it would be defined for now as rejecting if preventDefault() wasn't called or prompt() was already called.
> Having prompt() return a promise will let the developer know if the call > actually succeeded. As is, the call will fail silently which is generally a bad > thing. Furthermore, some UAs might want to reject the prompt request in some > situations so it wouldn't be a bad thing to let this door open in the API even > if it would be defined for now as rejecting if preventDefault() wasn't called or > prompt() was already called. I understand now, this makes more sense. I've made prompt() return a promise. I've also sent an email to blink-dev regarding these changes. Thanks for the guidance here.
https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... Source/modules/app_banner/BeforeInstallPromptEvent.cpp:67: ScriptPromise promise = resolver->promise(); You probably don't need a |resolver| or a |promise| here. You could simply call ScriptPromise::cast() and ScriptPromise::reject(). https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... Source/modules/app_banner/BeforeInstallPromptEvent.cpp:73: userChoice(scriptState); Why do you call that? https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... Source/modules/app_banner/BeforeInstallPromptEvent.cpp:75: resolver->reject(); I think you should reject with an InvalidState error. https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: [CallWith=ScriptState] Promise<DOMString> prompt(); Why Promise<DOMString>?
On 2015/06/11 00:42:17, Mounir Lamouri wrote: > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > File Source/modules/app_banner/BeforeInstallPromptEvent.cpp (right): > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > Source/modules/app_banner/BeforeInstallPromptEvent.cpp:67: ScriptPromise promise > = resolver->promise(); > You probably don't need a |resolver| or a |promise| here. You could simply call > ScriptPromise::cast() and ScriptPromise::reject(). This is easier. Thanks! > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > Source/modules/app_banner/BeforeInstallPromptEvent.cpp:73: > userChoice(scriptState); > Why do you call that? Thanks, I misunderstood the control flow here, so I'm removing this statement. > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: > [CallWith=ScriptState] Promise<DOMString> prompt(); > Why Promise<DOMString>? Should it be something different? I was following the userChoice promise here.
On 2015/06/11 at 05:40:31, dominickn wrote: > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > > File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): > > > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > > Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: > > [CallWith=ScriptState] Promise<DOMString> prompt(); > > Why Promise<DOMString>? > > Should it be something different? I was following the userChoice promise here. Promise<DOMString> means that prompt() will return a Promise object containing a DOMString. In other words: window.onbeforeinstallprompt(function(e) { e.preventDefault(); e.prompt().then(function(result) { typeof(e) === string; // true with your code }); }); The question is what do you want to pass as a result? It doesn't seem like anything need to be passed or am I missing something? Actually, what is the expected behaviour in order to get the user choice after the prompt? I guess the userChoice() call should be resolved after prompt() is called and the user made a choice?
On 2015/06/11 16:39:58, Mounir Lamouri wrote: > On 2015/06/11 at 05:40:31, dominickn wrote: > > > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > > > File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): > > > > > > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > > > Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: > > > [CallWith=ScriptState] Promise<DOMString> prompt(); > > > Why Promise<DOMString>? > > > > Should it be something different? I was following the userChoice promise here. > > Promise<DOMString> means that prompt() will return a Promise object containing a > DOMString. In other words: > window.onbeforeinstallprompt(function(e) { > e.preventDefault(); > e.prompt().then(function(result) { > typeof(e) === string; // true with your code > }); > }); > > The question is what do you want to pass as a result? It doesn't seem like > anything need to be passed or am I missing something? > Actually, what is the expected behaviour in order to get the user choice after > the prompt? I guess the userChoice() call should be resolved after prompt() is > called and the user made a choice? I can't imagine that you would need to ever do much in this promise. The pattern that I expect most developers will follow is to set up userChoice in initial event, then e.preventDefault and then cache the `e` some where so that they will later call `prompt` on some user gesture. Because I have access to the resoltion of userChoice it's not like that I need anything else for analytics or anything. Side question. User cancels prompt, and in the userChoice promise you call prompt again. What will happen? Will the prompt promise throw?
On 2015/06/11 22:03:20, PaulKinlan wrote: > On 2015/06/11 16:39:58, Mounir Lamouri wrote: > > On 2015/06/11 at 05:40:31, dominickn wrote: > > > > > > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > > > > File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): > > > > > > > > > > > https://codereview.chromium.org/1160413004/diff/60001/Source/modules/app_bann... > > > > Source/modules/app_banner/BeforeInstallPromptEvent.idl:12: > > > > [CallWith=ScriptState] Promise<DOMString> prompt(); > > > > Why Promise<DOMString>? > > > > > > Should it be something different? I was following the userChoice promise > here. > > > > Promise<DOMString> means that prompt() will return a Promise object containing > a > > DOMString. In other words: > > window.onbeforeinstallprompt(function(e) { > > e.preventDefault(); > > e.prompt().then(function(result) { > > typeof(e) === string; // true with your code > > }); > > }); > > > > The question is what do you want to pass as a result? It doesn't seem like > > anything need to be passed or am I missing something? > > Actually, what is the expected behaviour in order to get the user choice after > > the prompt? I guess the userChoice() call should be resolved after prompt() is > > called and the user made a choice? > > I can't imagine that you would need to ever do much in this promise. The > pattern that I expect > most developers will follow is to set up userChoice in initial event, then > e.preventDefault and then > cache the `e` some where so that they will later call `prompt` on some user > gesture. > > Because I have access to the resoltion of userChoice it's not like that I need > anything else for > analytics or anything. > > Side question. User cancels prompt, and in the userChoice promise you call > prompt again. What will happen? > Will the prompt promise throw? Perhaps instead of returning a promise, prompt() should simply throw (InvalidStateError?) if it is called incorrectly. That is, throw if the event hasn't yet triggered, if preventDefault() wasn't called, or if prompt() has already been called.
On 2015/06/12 at 00:24:06, dominickn wrote: > On 2015/06/11 22:03:20, PaulKinlan wrote: > > I can't imagine that you would need to ever do much in this promise. The > > pattern that I expect > > most developers will follow is to set up userChoice in initial event, then > > e.preventDefault and then > > cache the `e` some where so that they will later call `prompt` on some user > > gesture. > > > > Because I have access to the resoltion of userChoice it's not like that I need > > anything else for > > analytics or anything. > > > > Side question. User cancels prompt, and in the userChoice promise you call > > prompt again. What will happen? > > Will the prompt promise throw? > > Perhaps instead of returning a promise, prompt() should simply throw (InvalidStateError?) if it is called incorrectly. That is, throw if the event hasn't yet triggered, if preventDefault() wasn't called, or if prompt() has already been called. I don't think you should throw directly. Throwing is a last resort and I think there is enough benefit in having an async behaviour here. We can reject the Promise though.
On 2015/06/12 18:29:31, Mounir Lamouri wrote: > I don't think you should throw directly. Throwing is a last resort and I think > there is enough benefit in having an async behaviour here. We can reject the > Promise though. Thanks for the feedback, prompt() now returns a Promise<void>, and I'm now using ::cast and ::rejectWithDOMException.
Thanks! It looks good. Could you link the Chromium CL in the description and add me as reviewer for it? I want to make sure that there are no reasons why Chromium/Chrome would reject the request. https://codereview.chromium.org/1160413004/diff/80001/Source/modules/app_bann... File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): https://codereview.chromium.org/1160413004/diff/80001/Source/modules/app_bann... Source/modules/app_banner/BeforeInstallPromptEvent.idl:11: nit: no new line
I'd prefer this CL land before I submit the Chromium CL (I need to override the resolveEventRedispatch method, so it won't compile on trybots until this lands). I'll add you as a reviewer for the Chromium CL once it's in. https://codereview.chromium.org/1160413004/diff/80001/Source/modules/app_bann... File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): https://codereview.chromium.org/1160413004/diff/80001/Source/modules/app_bann... Source/modules/app_banner/BeforeInstallPromptEvent.idl:11: On 2015/06/17 16:27:37, Mounir Lamouri wrote: > nit: no new line Done.
On 2015/06/17 at 17:50:40, dominickn wrote: > I'd prefer this CL land before I submit the Chromium CL (I need to override the resolveEventRedispatch method, so it won't compile on trybots until this lands). I'll add you as a reviewer for the Chromium CL once it's in. > > https://codereview.chromium.org/1160413004/diff/80001/Source/modules/app_bann... > File Source/modules/app_banner/BeforeInstallPromptEvent.idl (right): > > https://codereview.chromium.org/1160413004/diff/80001/Source/modules/app_bann... > Source/modules/app_banner/BeforeInstallPromptEvent.idl:11: > On 2015/06/17 16:27:37, Mounir Lamouri wrote: > > nit: no new line > > Done. The Chromium CL doesn't have to land or even be reviewed, I just want to have a look. It's hard to evaluate whether the Blink public interface you added is correct without seeing how it is being used.
On 2015/06/17 17:53:27, Mounir Lamouri wrote: > The Chromium CL doesn't have to land or even be reviewed, I just want to have a > look. It's hard to evaluate whether the Blink public interface you added is > correct without seeing how it is being used. Fair enough. I've added the CL with you as a reviewer: https://codereview.chromium.org/1173293006
With the method rename it lgtm. However, we need to make sure that userChoice() works as expected: userChoice() should always resolve with something if the banner was shown, even if it was shown because of a prompt() after a preventDefault(). Though, from my understanding of the code, this is something that Chromium should take care of, not Blink. Also, when this lands, could you add test support in test_runner then land some tests in Blink? https://codereview.chromium.org/1160413004/diff/100001/public/platform/module... File public/platform/modules/app_banner/WebAppBannerClient.h (right): https://codereview.chromium.org/1160413004/diff/100001/public/platform/module... public/platform/modules/app_banner/WebAppBannerClient.h:23: virtual void resolveEventRedispatch(int requestId) { } I would rename that showAppBanner()
The CQ bit was checked by dominickn@google.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160413004/100001
The CQ bit was unchecked by dominickn@google.com
The CQ bit was checked by dominickn@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1160413004/#ps120001 (title: "Renaming method per reviewer feedback")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160413004/120001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/linux_chromium_gn_rel/bu...) mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59317)
The CQ bit was unchecked by dominickn@google.com
The CQ bit was checked by dominickn@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mlamouri@chromium.org Link to the patchset: https://codereview.chromium.org/1160413004/#ps140001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160413004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (JOB_FAILED, http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59353)
On 2015/06/18 00:45:53, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > mac_blink_rel on tryserver.blink (JOB_FAILED, > http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59353) The mac_blink_rel tests are a bit frustrating. Running the set of tests which fail on the bots on my Mac, I get some number of failures ranging from 0 to 4. It's rare for there to be 0 failures.
On 2015/06/18 at 19:03:39, dominickn wrote: > On 2015/06/18 00:45:53, commit-bot: I haz the power wrote: > > Try jobs failed on following builders: > > mac_blink_rel on tryserver.blink (JOB_FAILED, > > http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59353) > > The mac_blink_rel tests are a bit frustrating. Running the set of tests which fail on the bots on my Mac, I get some number of failures ranging from 0 to 4. It's rare for there to be 0 failures. Is that linked to your CL? Maybe the test is flaky regardless of your change?
On 2015/06/18 21:59:54, Mounir Lamouri wrote: > On 2015/06/18 at 19:03:39, dominickn wrote: > > On 2015/06/18 00:45:53, commit-bot: I haz the power wrote: > > > Try jobs failed on following builders: > > > mac_blink_rel on tryserver.blink (JOB_FAILED, > > > > http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/59353) > > > > The mac_blink_rel tests are a bit frustrating. Running the set of tests which > fail on the bots on my Mac, I get some number of failures ranging from 0 to 4. > It's rare for there to be 0 failures. > > Is that linked to your CL? Maybe the test is flaky regardless of your change? The tests aren't linked to my CL as far as I can tell. Rolling back my CL and running the tests gets similar failures on my machine.
Additionally, all tests passed before I added the change renaming the method.
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1160413004/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://src.chromium.org/viewvc/blink?view=rev&revision=197461 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
