Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2205523002:
AutoDiscardable property support on Chrome Extensions Tabs API. (Closed)
|
Created:
4 years, 4 months ago by Anderson Silva Modified:
4 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutoDiscardable property support on Chrome Extensions Tabs API.
BUG=621070
Committed: https://crrev.com/fd947a4ae3079420cbb1cdd374f7df188d023c41
Cr-Commit-Position: refs/heads/master@{#411641}
Patch Set 1 #
Total comments: 10
Patch Set 2 : nits fixed #
Total comments: 11
Patch Set 3 : resolved comments #Patch Set 4 : change javascript test #
Total comments: 8
Patch Set 5 : fixed nits #Messages
Total messages: 42 (21 generated)
The CQ bit was checked by andersoncss@google.com 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...
Description was changed from ========== AutoDiscardable property support on Chrome Extensions Tabs API. BUG=621070 ========== to ========== AutoDiscardable property support on Chrome Extensions Tabs API. BUG=621070 ==========
andersoncss@google.com changed reviewers: + chrisha@chromium.org, georgesak@chromium.org
Ptal, this should be the last major CL for the API.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm other than nits! https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:55: "autoDiscardable": {"type": "boolean", "description": "Wether the tab is auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low."}, Whether* ... https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:55: "autoDiscardable": {"type": "boolean", "description": "Wether the tab is auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low."}, An* auto-discardable ... https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:460: "description": "Wether the tabs are auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low." Whether* ... An* auto-discardable ... https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:618: "description": "Wether the tab should be auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low." Whether* ... An* ... https://codereview.chromium.org/2205523002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:49: function resetAutoDiscardable() { Another +2 indent here as well?
https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:55: "autoDiscardable": {"type": "boolean", "description": "Wether the tab is auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low."}, On 2016/08/02 14:40:06, chrisha (slow) wrote: > Whether* ... Done. https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:55: "autoDiscardable": {"type": "boolean", "description": "Wether the tab is auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low."}, On 2016/08/02 14:40:06, chrisha (slow) wrote: > An* auto-discardable ... Done. https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:460: "description": "Wether the tabs are auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low." On 2016/08/02 14:40:06, chrisha (slow) wrote: > Whether* ... An* auto-discardable ... Done. https://codereview.chromium.org/2205523002/diff/1/chrome/common/extensions/ap... chrome/common/extensions/api/tabs.json:618: "description": "Wether the tab should be auto-discardable. A auto-discardable tab is eligible for discarding when memory resources are low." On 2016/08/02 14:40:06, chrisha (slow) wrote: > Whether* ... An* ... Done. https://codereview.chromium.org/2205523002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:49: function resetAutoDiscardable() { On 2016/08/02 14:40:06, chrisha (slow) wrote: > Another +2 indent here as well? Done.
andersoncss@google.com changed reviewers: + rdevlin.cronin@chromium.org
Hi Devlin, Can you please take a look at this CL? It should be our last major CL for the API. Thanks
overall, pretty good. Just a couple nits. https://codereview.chromium.org/2205523002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:1623: // Update the auto-discardable state of the first tab. nit: isn't this technically the second tab? Maybe just "of web contents A". https://codereview.chromium.org/2205523002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/common/extension... chrome/common/extensions/api/tabs.json:55: "autoDiscardable": {"type": "boolean", "description": "Whether the tab is auto-discardable. An auto-discardable tab is eligible for discarding when memory resources are low."}, Saying "whether the tab is auto-discardable. <definition of auto-discardable>" could probably be replaced here with "whether the tab is <definition of auto-discardable>". Also, aren't non-auto-discardable tabs eligible for discarding from the discard() method? If so, maybe we should change this to something like "can be discarded automatically by the browser when resources are low." https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:23: function setNonAutoDiscardable() { Can we combine this with the other discardable test?
https://codereview.chromium.org/2205523002/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/api/tabs/tabs_test.cc (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/browser/extensio... chrome/browser/extensions/api/tabs/tabs_test.cc:1623: // Update the auto-discardable state of the first tab. On 2016/08/02 22:48:14, Devlin wrote: > nit: isn't this technically the second tab? Maybe just "of web contents A". Done. https://codereview.chromium.org/2205523002/diff/20001/chrome/common/extension... File chrome/common/extensions/api/tabs.json (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/common/extension... chrome/common/extensions/api/tabs.json:55: "autoDiscardable": {"type": "boolean", "description": "Whether the tab is auto-discardable. An auto-discardable tab is eligible for discarding when memory resources are low."}, On 2016/08/02 22:48:14, Devlin wrote: > Saying "whether the tab is auto-discardable. <definition of auto-discardable>" > could probably be replaced here with "whether the tab is <definition of > auto-discardable>". Also, aren't non-auto-discardable tabs eligible for > discarding from the discard() method? If so, maybe we should change this to > something like "can be discarded automatically by the browser when resources are > low." Yes, you're right, non-auto-discardable tabs can be discarded by discard(). I'll update the description. https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:23: function setNonAutoDiscardable() { On 2016/08/02 22:48:14, Devlin wrote: > Can we combine this with the other discardable test? You mean do set and reset (both update() calls) in the same function? Or test this in tabs_test.cc? We can't really test a onUpdated event notification on tabs_test.cc (or we didn't find a way to do so).
Sorry for the delay; was OOO. https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:23: function setNonAutoDiscardable() { On 2016/08/03 14:47:41, Anderson Silva wrote: > On 2016/08/02 22:48:14, Devlin wrote: > > Can we combine this with the other discardable test? > > You mean do set and reset (both update() calls) in the same function? > Or test this in tabs_test.cc? We can't really test a onUpdated event > notification on tabs_test.cc (or we didn't find a way to do so). I meant adding this to chrome/test/data/extensions/api_test/tabs/basics/discarded.js in a separate function to reduce boilerplate.
https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:23: function setNonAutoDiscardable() { On 2016/08/10 19:25:12, Devlin wrote: > On 2016/08/03 14:47:41, Anderson Silva wrote: > > On 2016/08/02 22:48:14, Devlin wrote: > > > Can we combine this with the other discardable test? > > > > You mean do set and reset (both update() calls) in the same function? > > Or test this in tabs_test.cc? We can't really test a onUpdated event > > notification on tabs_test.cc (or we didn't find a way to do so). > > I meant adding this to > chrome/test/data/extensions/api_test/tabs/basics/discarded.js in a separate > function to reduce boilerplate. Hmm, I think it would work as well doing it altogether but they are different properties and being so I would prefer to keep them separate. Let me know.
https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:23: function setNonAutoDiscardable() { On 2016/08/10 20:30:53, Anderson Silva wrote: > On 2016/08/10 19:25:12, Devlin wrote: > > On 2016/08/03 14:47:41, Anderson Silva wrote: > > > On 2016/08/02 22:48:14, Devlin wrote: > > > > Can we combine this with the other discardable test? > > > > > > You mean do set and reset (both update() calls) in the same function? > > > Or test this in tabs_test.cc? We can't really test a onUpdated event > > > notification on tabs_test.cc (or we didn't find a way to do so). > > > > I meant adding this to > > chrome/test/data/extensions/api_test/tabs/basics/discarded.js in a separate > > function to reduce boilerplate. > > Hmm, I think it would work as well doing it altogether but they are different > properties and being so I would prefer to keep them separate. > Let me know. Extension javascript tests are designed to be able to run multiple tests in a single browsertest, kind of like how we can do multiple assertions. chrome.test.runTests() runs all functions it's passed, and each function is considered a separate test. So what I'm recommending is keeping the tests distinct, but running them in the same JS file and browsertest executable in order to cut down on boilerplate and speed up browsertest execution time. Does that make sense?
https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:23: function setNonAutoDiscardable() { On 2016/08/10 20:45:49, Devlin wrote: > On 2016/08/10 20:30:53, Anderson Silva wrote: > > On 2016/08/10 19:25:12, Devlin wrote: > > > On 2016/08/03 14:47:41, Anderson Silva wrote: > > > > On 2016/08/02 22:48:14, Devlin wrote: > > > > > Can we combine this with the other discardable test? > > > > > > > > You mean do set and reset (both update() calls) in the same function? > > > > Or test this in tabs_test.cc? We can't really test a onUpdated event > > > > notification on tabs_test.cc (or we didn't find a way to do so). > > > > > > I meant adding this to > > > chrome/test/data/extensions/api_test/tabs/basics/discarded.js in a separate > > > function to reduce boilerplate. > > > > Hmm, I think it would work as well doing it altogether but they are different > > properties and being so I would prefer to keep them separate. > > Let me know. > > Extension javascript tests are designed to be able to run multiple tests in a > single browsertest, kind of like how we can do multiple assertions. > chrome.test.runTests() runs all functions it's passed, and each function is > considered a separate test. So what I'm recommending is keeping the tests > distinct, but running them in the same JS file and browsertest executable in > order to cut down on boilerplate and speed up browsertest execution time. Does > that make sense? (here's an example: https://cs.chromium.org/chromium/src/chrome/test/data/extensions/api_test/coo...)
https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js (right): https://codereview.chromium.org/2205523002/diff/20001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/autoDiscardable.js:23: function setNonAutoDiscardable() { On 2016/08/10 20:45:49, Devlin wrote: > On 2016/08/10 20:30:53, Anderson Silva wrote: > > On 2016/08/10 19:25:12, Devlin wrote: > > > On 2016/08/03 14:47:41, Anderson Silva wrote: > > > > On 2016/08/02 22:48:14, Devlin wrote: > > > > > Can we combine this with the other discardable test? > > > > > > > > You mean do set and reset (both update() calls) in the same function? > > > > Or test this in tabs_test.cc? We can't really test a onUpdated event > > > > notification on tabs_test.cc (or we didn't find a way to do so). > > > > > > I meant adding this to > > > chrome/test/data/extensions/api_test/tabs/basics/discarded.js in a separate > > > function to reduce boilerplate. > > > > Hmm, I think it would work as well doing it altogether but they are different > > properties and being so I would prefer to keep them separate. > > Let me know. > > Extension javascript tests are designed to be able to run multiple tests in a > single browsertest, kind of like how we can do multiple assertions. > chrome.test.runTests() runs all functions it's passed, and each function is > considered a separate test. So what I'm recommending is keeping the tests > distinct, but running them in the same JS file and browsertest executable in > order to cut down on boilerplate and speed up browsertest execution time. Does > that make sense? Oh, I see you mean now. It's basically using the same functions within only one file. I thought I was supposed to mix up the functions. I'll redo it then. Thanks
The CQ bit was checked by andersoncss@google.com to run a CQ dry run
changed both javascript tests into one
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_...)
lgtm https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (left): https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. To make the diff a little cleaner, can we keep the test files named discarded.* rather than discarding.*? https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/discarding.js (right): https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarding.js:83: function (tabId, changeInfo, tab) { nit: no space after function https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarding.js:97: pass(function (tab) { nit: no space after function https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarding.js:109: function (tabId, changeInfo, tab) { ditto
Patchset #6 (id:100001) has been deleted
Patchset #5 (id:80001) has been deleted
fixed nits, sending to CQ Thanks! https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/discarded.js (left): https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarded.js:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/08/11 16:45:54, Devlin wrote: > To make the diff a little cleaner, can we keep the test files named discarded.* > rather than discarding.*? Done. https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/tabs/basics/discarding.js (right): https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarding.js:83: function (tabId, changeInfo, tab) { On 2016/08/11 16:45:54, Devlin wrote: > nit: no space after function Done. https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarding.js:97: pass(function (tab) { On 2016/08/11 16:45:54, Devlin wrote: > nit: no space after function Done. https://codereview.chromium.org/2205523002/diff/60001/chrome/test/data/extens... chrome/test/data/extensions/api_test/tabs/basics/discarding.js:109: function (tabId, changeInfo, tab) { On 2016/08/11 16:45:54, Devlin wrote: > ditto Done.
The CQ bit was checked by andersoncss@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from chrisha@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2205523002/#ps120001 (title: "fixed nits")
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: 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 andersoncss@google.com
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: 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 andersoncss@google.com
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.
Description was changed from ========== AutoDiscardable property support on Chrome Extensions Tabs API. BUG=621070 ========== to ========== AutoDiscardable property support on Chrome Extensions Tabs API. BUG=621070 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== AutoDiscardable property support on Chrome Extensions Tabs API. BUG=621070 ========== to ========== AutoDiscardable property support on Chrome Extensions Tabs API. BUG=621070 Committed: https://crrev.com/fd947a4ae3079420cbb1cdd374f7df188d023c41 Cr-Commit-Position: refs/heads/master@{#411641} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/fd947a4ae3079420cbb1cdd374f7df188d023c41 Cr-Commit-Position: refs/heads/master@{#411641} |
