|
|
DescriptionChange getAccessToken and getExtensionPrinterAccess to sendWithPromise
Change getAccessToken and getExtensionPrinterAccess to sendWithPromise
in print preview.
BUG=717296
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2938073003
Cr-Commit-Position: refs/heads/master@{#481451}
Committed: https://chromium.googlesource.com/chromium/src/+/699c1708fea286497554a012970ddb99a4123c91
Patch Set 1 #Patch Set 2 : Cleanup #
Total comments: 6
Patch Set 3 : Address comments #
Total comments: 10
Patch Set 4 : Address comments #
Total comments: 8
Patch Set 5 : Comments #Patch Set 6 : Remove map and extra dest type #
Total comments: 6
Patch Set 7 : Address comments, remove extra code #
Total comments: 4
Patch Set 8 : Remove queue #
Total comments: 2
Patch Set 9 : Fix annotation #
Messages
Total messages: 61 (46 generated)
Description was changed from ========== Change getAccessToken and getExtensionPrinterAccess to sendWithPromise Change getAccessToken and getExtensionPrinterAccess to sendWithPromise in print preview. BUG=717296 ========== to ========== Change getAccessToken and getExtensionPrinterAccess to sendWithPromise Change getAccessToken and getExtensionPrinterAccess to sendWithPromise in print preview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@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...
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
The CQ bit was checked by rbpotter@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...
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1018: this.handleProvisionalDestinationResolved_.bind(this, id), handleProvisionalDestinationResolved_and handleProvisionalDestinationRejected_ are only used here. Can we inline them here instead? https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1149: this.nativeLayer_.getPrivetPrinters().then( Unrelated to this CL, but related to previous. Is |waitForRegisterDestination_| and |waitForRegister| still relevant? We already have a Promise for it. Could it be something like waitForRegister: function(id) { return this.nativeLayer_.getPrivetPrinters().then( this.endPrivetPrinterSearch_.bind(this)); } And whoever calls this, uses the returned Promise? https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1173: if (!args->GetString(0, &callback_id) || !args->GetString(1, &type)) { Can this happen other than programmer's error? Should this be a CHECK/DCHECK instead?
The CQ bit was checked by rbpotter@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...
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1018: this.handleProvisionalDestinationResolved_.bind(this, id), On 2017/06/15 22:20:13, dpapad wrote: > handleProvisionalDestinationResolved_and handleProvisionalDestinationRejected_ > are only used here. Can we inline them here instead? Done. https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1149: this.nativeLayer_.getPrivetPrinters().then( On 2017/06/15 22:20:13, dpapad wrote: > Unrelated to this CL, but related to previous. Is |waitForRegisterDestination_| > and |waitForRegister| still relevant? We already have a Promise for it. Could it > be something like > > waitForRegister: function(id) { > return this.nativeLayer_.getPrivetPrinters().then( > this.endPrivetPrinterSearch_.bind(this)); > } > > And whoever calls this, uses the returned Promise? I think the issue is that the promise returned by getPrivetPrinters() is not resolved until privet search times out, i.e. "all" privet destinations have been retrieved. waitForRegisterDestination_ is used so that the desired destination that is being registered can be selected as soon as it arrives, see onPrivetPrinterAdded_ here: (https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/d...). I think to eliminate these variables we would need to create a new native layer function, like getPrivetPrinter(), that would send the specific destination ID to C++ so that the promise could be resolved when that destination was returned, and would be resolved with the appropriate structure (instead of just base::Value()). Then we would have: nativeLayer_.getPrivetPrinter(id).then( this.onPrivetPrinterAdded_().bind(this)); Do you think this is worth doing (maybe in a follow-up CL)? https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2938073003/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1173: if (!args->GetString(0, &callback_id) || !args->GetString(1, &type)) { On 2017/06/15 22:20:13, dpapad wrote: > Can this happen other than programmer's error? Should this be a CHECK/DCHECK > instead? Done.
https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1019: * Removes the destination from the store and replaces it with a Nit (Optional): Given that this function is now inlined, maybe move this explanatory within the function itself? https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1028: this.removeProvisionalDestination_(provisionalId); provisionalId parameter no longer needed. s/provisionalId/id or s/provisionalId/destination.id See related comment below. https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1034: }.bind(this, id), This could be simply ".bind(this)" now. There is no need to pass the id along, because it is already in the function's scope. https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1045: }.bind(this, id)); Same here. https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:481: handler_->SendRequestInProgress(callback_id); Trying to understand the need for this. Per my understanding, if from JS there are two calls as follows getAccessToken(someType).then(...) ... getAccessToken(someType).then(...) The 2nd one will get back a Promise that is resolved immediately with an empty response, where as the 1st one will get resolved sometime later when the actual response is ready? I would expect that both calls would get the same response, at the same time (when the token for someType is available). Is my understanding correct?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@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...
https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1019: * Removes the destination from the store and replaces it with a On 2017/06/16 03:00:59, dpapad wrote: > Nit (Optional): Given that this function is now inlined, maybe move this > explanatory within the function itself? Done. https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1028: this.removeProvisionalDestination_(provisionalId); On 2017/06/16 03:01:00, dpapad wrote: > provisionalId parameter no longer needed. > > s/provisionalId/id or > s/provisionalId/destination.id > > See related comment below. Done. https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1034: }.bind(this, id), On 2017/06/16 03:00:59, dpapad wrote: > This could be simply ".bind(this)" now. There is no need to pass the id along, > because it is already in the function's scope. Done. https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1045: }.bind(this, id)); On 2017/06/16 03:01:00, dpapad wrote: > Same here. Done. https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2938073003/diff/60001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:481: handler_->SendRequestInProgress(callback_id); On 2017/06/16 03:01:00, dpapad wrote: > Trying to understand the need for this. Per my understanding, if from JS there > are two calls as follows > > getAccessToken(someType).then(...) > ... > getAccessToken(someType).then(...) > The 2nd one will get back a Promise that is resolved immediately with an empty > response, where as the 1st one will get resolved sometime later when the actual > response is ready? > > I would expect that both calls would get the same response, at the same time > (when the token for someType is available). Is my understanding correct? Replaced with check in JS to ensure only one call is made to getAccessToken for each origin per offline discussion.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/cloud_print_interface.js:109: this.accessTokenRequestMap_ = new Map(); Let's wait for @vitalybuka's response to see if this Map is necessary. https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/cloud_print_interface.js:423: request.origin, Is this a string or a print_preview.DestinationOrigin? Shouldn't the map be typed as !Map<!print_preview.DestinationOrigin, !Promise> instead? https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1018: /** This comment needs +2 indent. https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1185: function(el, i, destinations) { Do you need to pass |i| and |destinations| since they are not used? Did the compiler complain?
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by rbpotter@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...
https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/cloud_print_interface.js:109: this.accessTokenRequestMap_ = new Map(); On 2017/06/16 22:21:29, dpapad wrote: > Let's wait for @vitalybuka's response to see if this Map is necessary. Done. Removed as it seems the DestinationOrigin.PROFILE feature was never shipped and has an unclear future. https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/cloud_print_interface.js:423: request.origin, On 2017/06/16 22:21:29, dpapad wrote: > Is this a string or a print_preview.DestinationOrigin? Shouldn't the map be > typed as > > !Map<!print_preview.DestinationOrigin, !Promise> instead? Done. https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1018: /** On 2017/06/16 22:21:30, dpapad wrote: > This comment needs +2 indent. Done. https://codereview.chromium.org/2938073003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:1185: function(el, i, destinations) { On 2017/06/16 22:21:30, dpapad wrote: > Do you need to pass |i| and |destinations| since they are not used? Did the > compiler complain? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:394: this.accessTokenRequestPromise_ = this.accessTokenRequestPromise_ || A bit confused by this line. Is the intention to be equivalent to the following? if (accessTokenRequestPromise_ != null) return; this.accessTokenRequestPromise_ = this.nativeLayer_.getAccessToken(request.origin).then(..); https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:477: assert(authType == print_preview.DestinationOrigin.DEVICE); Do we need those assertions (here and line 479 below) and the TODO? https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination_store.js:999: * @param {!print_preview.ProvisionalDestinationInfo} The @param comment should still be before the "function() {".
https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:394: this.accessTokenRequestPromise_ = this.accessTokenRequestPromise_ || On 2017/06/20 01:36:21, dpapad wrote: > A bit confused by this line. Is the intention to be equivalent to the following? > > if (accessTokenRequestPromise_ != null) > return; > > this.accessTokenRequestPromise_ = > this.nativeLayer_.getAccessToken(request.origin).then(..); Yes, that was the intent. That is much clearer, so changed. https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:477: assert(authType == print_preview.DestinationOrigin.DEVICE); On 2017/06/20 01:36:21, dpapad wrote: > Do we need those assertions (here and line 479 below) and the TODO? Removed assertions. Also removed authType from function since it is not used any more. https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2938073003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination_store.js:999: * @param {!print_preview.ProvisionalDestinationInfo} On 2017/06/20 01:36:21, dpapad wrote: > The @param comment should still be before the "function() {". Done.
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: closure_compilation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/closure_compila...)
https://codereview.chromium.org/2938073003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:391: return this.sendRequest_(request); This method is not declaring any @return. Should this be if (request.origin == print_preview.DestinationOrigin.COOKIES) { this.sendRequest_(request); return; } https://codereview.chromium.org/2938073003/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:393: this.requestQueue_.push(request); So, is the requestQueue_ necessary anymore? I think we can remove as follows, WDYT? Per my understanding this was used to queue a print request, until the ACCESS_TOKEN_READY event was fired. Once fired, the list was being searched for the correct authType (not relevant anymore) and a request was sent to the cloud print API. Now that we have the Promise, we don't need to queue, all we need to do is to wait for the Promise. sendOrQueueRequest_: function(request) { if (request.origin == print_preview.DestinationOrigin.COOKIES) { this.sendRequest_(request); return; } if (this.accessTokenRequestPromise_ == null) { this.accessTokenRequestPromise_ = this.nativeLayer_.getAccessToken(request.origin); } this.accessTokenRequestPromise_.then( this.onAccessTokenReady_.bind(this, request)); } onAccessTokenReady_: function (request, accessToken) { if (accessToken) { ... } else { ... } this.accessTokenRequestPromise_ = null; }
The CQ bit was checked by rbpotter@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2938073003/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:391: return this.sendRequest_(request); On 2017/06/20 16:09:09, dpapad wrote: > This method is not declaring any @return. Should this be > > if (request.origin == print_preview.DestinationOrigin.COOKIES) { > this.sendRequest_(request); > return; > } I suspect this was done since sendRequest_ does not return anything, but agree this is more clear. Done. https://codereview.chromium.org/2938073003/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:393: this.requestQueue_.push(request); On 2017/06/20 16:09:09, dpapad wrote: > So, is the requestQueue_ necessary anymore? I think we can remove as follows, > WDYT? > > Per my understanding this was used to queue a print request, until the > ACCESS_TOKEN_READY event was fired. Once fired, the list was being searched for > the correct authType (not relevant anymore) and a request was sent to the cloud > print API. > > Now that we have the Promise, we don't need to queue, all we need to do is to > wait for the Promise. > > sendOrQueueRequest_: function(request) { > if (request.origin == print_preview.DestinationOrigin.COOKIES) { > this.sendRequest_(request); > return; > } > > if (this.accessTokenRequestPromise_ == null) { > this.accessTokenRequestPromise_ = > this.nativeLayer_.getAccessToken(request.origin); > } > > this.accessTokenRequestPromise_.then( > this.onAccessTokenReady_.bind(this, request)); > } > > onAccessTokenReady_: function (request, accessToken) { > if (accessToken) { > ... > } else { > ... > } > this.accessTokenRequestPromise_ = null; > } Done. Queue is not needed anymore, thanks for the suggestion.
LGTM https://codereview.chromium.org/2938073003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:100: * @private {?Promise} I believe this is now ?Promise<string>
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
https://codereview.chromium.org/2938073003/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/cloud_print_interface.js (right): https://codereview.chromium.org/2938073003/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/cloud_print_interface.js:100: * @private {?Promise} On 2017/06/21 18:18:41, dpapad wrote: > I believe this is now ?Promise<string> Done.
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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2938073003/#ps180001 (title: "Fix annotation")
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": 180001, "attempt_start_ts": 1498106990225050, "parent_rev": "1eeccaf7268c04ca0f50c076c9ac827167b6735a", "commit_rev": "699c1708fea286497554a012970ddb99a4123c91"}
Message was sent while issue was closed.
Description was changed from ========== Change getAccessToken and getExtensionPrinterAccess to sendWithPromise Change getAccessToken and getExtensionPrinterAccess to sendWithPromise in print preview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Change getAccessToken and getExtensionPrinterAccess to sendWithPromise Change getAccessToken and getExtensionPrinterAccess to sendWithPromise in print preview. BUG=717296 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2938073003 Cr-Commit-Position: refs/heads/master@{#481451} Committed: https://chromium.googlesource.com/chromium/src/+/699c1708fea286497554a012970d... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/699c1708fea286497554a012970d... |