Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2231873002:
Added budget_service.mojom (Closed)
|
Created:
4 years, 4 months ago by harkness Modified:
4 years, 4 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, qsr+mojo_chromium.org, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@budget_api Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded budget_service.mojom
This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService.
This also adds a mock and a layout test to check the mojo service.
BUG=617971
Committed: https://crrev.com/7e00f5e7b1fdedd2a32ef8321bb13e2f0726bd83
Cr-Commit-Position: refs/heads/master@{#414265}
Patch Set 1 #
Total comments: 4
Patch Set 2 : code review comments #
Total comments: 2
Patch Set 3 : Added comments and converted scriptState passing to be by PassRefPtr #
Total comments: 1
Patch Set 4 : Fixed crash, all tests working #
Total comments: 41
Patch Set 5 : Code review comments - nits and modified scriptStatePassing #Patch Set 6 : Converted BudgetChunk dictionary to BudgetState interface and other code review updates #
Total comments: 26
Patch Set 7 : Split tests, cleaned up constructor, and nits #
Total comments: 10
Patch Set 8 : test cleanup #
Total comments: 5
Patch Set 9 : Add context safety check, add expected global interface #Patch Set 10 : Added interface to virtual/stable/webexposed #
Total comments: 2
Patch Set 11 : Guarded BudgetState interface with flag. #Patch Set 12 : Add a connection error handler to BudgetService #
Total comments: 2
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 63 (29 generated)
Description was changed from ========== Added budget_service.mojom BUG=617971 ========== to ========== This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. BUG=617971 ==========
harkness@chromium.org changed reviewers: + peter@chromium.org
Please reformat the CL's description to be in the "title, description, bug" format (otherwise it'll get lost when pushed to the git repo). What we generally try to do with mojom files, since they require security review, is to include the usage with the initial CL. Would it be too much work to add the calling side of the service within your module (i.e. connecting to it and making the calls), and having a layout test with a JavaScript implementation of the service provided? https://codereview.chromium.org/2231873002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom (right): https://codereview.chromium.org/2231873002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:16: // Time at which the budget is available. Please define what sort of value the double contains (... time in milliseconds, counted from January 1, 1970 ...) https://codereview.chromium.org/2231873002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:24: GetBudget(string origin) => (array<BudgetState> budget); Please use url.mojom.Origin
Description was changed from ========== This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. BUG=617971 ========== to ========== Added budget_service.mojom This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. BUG=617971 ==========
Description was changed from ========== Added budget_service.mojom This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. BUG=617971 ========== to ========== Added budget_service.mojom This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. This also adds a mock and a layout test to check the mojo service. BUG=617971 ==========
https://codereview.chromium.org/2231873002/diff/1/third_party/WebKit/public/p... File third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom (right): https://codereview.chromium.org/2231873002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:16: // Time at which the budget is available. On 2016/08/10 16:53:31, Peter Beverloo wrote: > Please define what sort of value the double contains (... time in milliseconds, > counted from January 1, 1970 ...) updated. https://codereview.chromium.org/2231873002/diff/1/third_party/WebKit/public/p... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:24: GetBudget(string origin) => (array<BudgetState> budget); On 2016/08/10 16:53:30, Peter Beverloo wrote: > Please use url.mojom.Origin Done. https://codereview.chromium.org/2231873002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:53: m_service->GetBudget(origin, convertToBaseCallback(WTF::bind(&BudgetService::asyncGetBudget, wrapPersistent(this), unretained(scriptState), wrapPersistent(resolver)))); I couldn't find documentation about when to use unretained() vs. the various wrap options. Is the use of unretained() safe if I check it in the return? Or do I need to do more work? Is this documented anywhere? https://codereview.chromium.org/2231873002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:73: budget[i] = freezeV8Object(toV8(chunk, scriptState), scriptState->isolate()); This line crashes, uploading so you can take a look.
https://codereview.chromium.org/2231873002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:54: m_service->GetBudget(origin, convertToBaseCallback(WTF::bind(&BudgetService::asyncGetBudget, wrapPersistent(this), PassRefPtr<ScriptState>(scriptState), wrapPersistent(resolver)))); I converted this from unretained to PassRefPtr after a discussion with John and some digging. Also, I did verify that the scriptState in asyncGetBudget does still have a valid origin set, so I think passing it is not what's causing the toV8 call to fail.
harkness@chromium.org changed reviewers: + johnme@chromium.org
Looks good, left some nits and questions about things I'm not sure of. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:8: const TEST_BUDGET_VALUE = 2.3; Nit: value -> available? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:9: const TEST_BUDGET_EXPIRATION = 3.4; Whilst 3 milliseconds past midnight 1970 is a valid date, it might be clearer to use a more realistic one. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:15: ).then(mojo => { Nit: please move the ] down next to the ) https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:16: let [budget_service, router] = mojo.modules; Nit: camelCase in JS https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:28: ScriptPromise BudgetService::getCost(ScriptState* scriptState, const AtomicString& /* actionType */) operationType? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:30: DCHECK(m_service); Nit: Perhaps just DCHECK once in the constructor? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:54: m_service->GetBudget(origin, convertToBaseCallback(WTF::bind(&BudgetService::asyncGetBudget, wrapPersistent(this), PassRefPtr<ScriptState>(scriptState), wrapPersistent(resolver)))); 1. Is it ok to hold a reference to the ScriptState, or could this cause a reference cycle? 2. Should this be adoptRef(scriptState) instead? 3. Is it valid to adopt a raw pointer which may have existing refs elsewhere? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:62: if (!scriptState) { How can this occur? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:80: budget[i] = freezeV8Object(toV8(chunk, scriptState.get()), scriptState->isolate()); BudgetState objects shouldn't need to be individually frozen, since all their attributes are readonly primitives (once you update BudgetChunk to match the spec and stop being a dictionary). This might mean you no longer need to deal with v8 directly, which would be nice. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.h (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.h:21: class BudgetService final : public GarbageCollectedFinalized<BudgetService>, public ScriptWrappable { Why does this need to be Finalized? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.h:37: void asyncGetCost(ScriptPromiseResolver*, double) const; Nit: gotCost or onGetCost? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/modules.gypi (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/modules.gypi:466: 'budget/BudgetChunk.idl', What's a BudgetChunk? The spec seems to call this interface BudgetState; though perhaps that's something for a different CL... https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py:65: resources_dir = self._filesystem.join(test_dir, "resources") What's the difference between this and imported_resourced_dir? Is there a reason not to use that? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:9: enum OperationType { Nit: Might need to be namespaced? e.g. BudgetOperationType? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:15: // Amount of budget that will be available. Please be more specific, e.g. "A lower bound for the minimum amount of budget available between this time and the previous time."
https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:8: const TEST_BUDGET_VALUE = 2.3; On 2016/08/17 13:54:01, johnme wrote: > Nit: value -> available? Done. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:9: const TEST_BUDGET_EXPIRATION = 3.4; On 2016/08/17 13:54:01, johnme wrote: > Whilst 3 milliseconds past midnight 1970 is a valid date, it might be clearer to > use a more realistic one. Done. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:15: ).then(mojo => { On 2016/08/17 13:54:01, johnme wrote: > Nit: please move the ] down next to the ) Done. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:16: let [budget_service, router] = mojo.modules; On 2016/08/17 13:54:01, johnme wrote: > Nit: camelCase in JS budget_service is the name of the mojo service, so this is actually consistent with that, even if not with normal JS style. The other mock I'd looked at used this casing, do you think it's better to go JS style route or Mojo consistency route? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:28: ScriptPromise BudgetService::getCost(ScriptState* scriptState, const AtomicString& /* actionType */) On 2016/08/17 13:54:02, johnme wrote: > operationType? Actually, when it comes in to this function, it's action type and needs to be mapped to operation type. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:30: DCHECK(m_service); On 2016/08/17 13:54:01, johnme wrote: > Nit: Perhaps just DCHECK once in the constructor? Done. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:54: m_service->GetBudget(origin, convertToBaseCallback(WTF::bind(&BudgetService::asyncGetBudget, wrapPersistent(this), PassRefPtr<ScriptState>(scriptState), wrapPersistent(resolver)))); On 2016/08/17 13:54:01, johnme wrote: > 1. Is it ok to hold a reference to the ScriptState, or could this cause a > reference cycle? > > 2. Should this be adoptRef(scriptState) instead? > > 3. Is it valid to adopt a raw pointer which may have existing refs elsewhere? I didn't know about adoptRef, and if I'd known I probably should have used that. However, in digging into this, it turns out that the scriptState is a member of the resolver. So I can just get the scriptState from there rather than passing it explicitly. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:62: if (!scriptState) { On 2016/08/17 13:54:02, johnme wrote: > How can this occur? That was left over from when I was passing ScriptState as unretained, so I was checking it. Given that I'm now getting scriptState from the resolver, I do need this, but I combined it with the check for contextIsValid on line 65. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:80: budget[i] = freezeV8Object(toV8(chunk, scriptState.get()), scriptState->isolate()); On 2016/08/17 13:54:01, johnme wrote: > BudgetState objects shouldn't need to be individually frozen, since all their > attributes are readonly primitives (once you update BudgetChunk to match the > spec and stop being a dictionary). This might mean you no longer need to deal > with v8 directly, which would be nice. I think I can get away without the extra freezeV8Object call, but I still think I need the toV8, and that's where I needed the extra scope. The difficulty here is that all other instances of FrozenArray returning an object are returning a reference to a member object of the Blink class, so they can rely on the class to control the lifetime. In my case, I need to explicitly allocate a copy of the array on the v8 heap because after that method returns, the memory that was passed from the browser will be deallocated. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.h (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.h:21: class BudgetService final : public GarbageCollectedFinalized<BudgetService>, public ScriptWrappable { On 2016/08/17 13:54:02, johnme wrote: > Why does this need to be Finalized? m_service requires it. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.h:37: void asyncGetCost(ScriptPromiseResolver*, double) const; On 2016/08/17 13:54:02, johnme wrote: > Nit: gotCost or onGetCost? Normally I hesititate on just changing verb tense, because I find it leads to strange namings, but this one does seem pretty straightforward. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/modules.gypi (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/modules.gypi:466: 'budget/BudgetChunk.idl', On 2016/08/17 13:54:02, johnme wrote: > What's a BudgetChunk? The spec seems to call this interface BudgetState; though > perhaps that's something for a different CL... Yeah, that's the drawback of submitting code before the spec was 100% nailed down. I'm slowly going through and sync'ing them up. That will be in the next CL. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py:65: resources_dir = self._filesystem.join(test_dir, "resources") On 2016/08/17 13:54:02, johnme wrote: > What's the difference between this and imported_resourced_dir? Is there a reason > not to use that? This was actually a change I got from Peter. Originally he tried using imported_resources_dir which didn't work, and then switched to this form. I don't know the details of the apache config, so I'm not sure why the original approach with imported_resources_dir didn't work. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/publ... File third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:9: enum OperationType { On 2016/08/17 13:54:02, johnme wrote: > Nit: Might need to be namespaced? e.g. BudgetOperationType? Good idea. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/publ... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:15: // Amount of budget that will be available. On 2016/08/17 13:54:02, johnme wrote: > Please be more specific, e.g. "A lower bound for the minimum amount of budget > available between this time and the previous time." Done.
https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:16: let [budget_service, router] = mojo.modules; On 2016/08/18 10:23:26, harkness wrote: > On 2016/08/17 13:54:01, johnme wrote: > > Nit: camelCase in JS > > budget_service is the name of the mojo service, so this is actually consistent > with that, even if not with normal JS style. The other mock I'd looked at used > this casing, do you think it's better to go JS style route or Mojo consistency > route? geolocation-mock.js calls one of its loaded mojo services permissionStatus, so consistency can go either way. Stick with JS style I guess? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:28: ScriptPromise BudgetService::getCost(ScriptState* scriptState, const AtomicString& /* actionType */) On 2016/08/18 10:23:26, harkness wrote: > On 2016/08/17 13:54:02, johnme wrote: > > operationType? > > Actually, when it comes in to this function, it's action type and needs to be > mapped to operation type. Oh, I was looking at https://beverloo.github.io/budget-api/ but I see that you already called it actionType in BudgetService.idl. Presumably later you'll rename it to match the spec in both places? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:80: budget[i] = freezeV8Object(toV8(chunk, scriptState.get()), scriptState->isolate()); On 2016/08/18 10:23:26, harkness wrote: > On 2016/08/17 13:54:01, johnme wrote: > > BudgetState objects shouldn't need to be individually frozen, since all their > > attributes are readonly primitives (once you update BudgetChunk to match the > > spec and stop being a dictionary). This might mean you no longer need to deal > > with v8 directly, which would be nice. > > I think I can get away without the extra freezeV8Object call, but I still think > I need the toV8, and that's where I needed the extra scope. > > The difficulty here is that all other instances of FrozenArray returning an > object are returning a reference to a member object of the Blink class, so they > can rely on the class to control the lifetime. In my case, I need to explicitly > allocate a copy of the array on the v8 heap because after that method returns, > the memory that was passed from the browser will be deallocated. It looks like ExtendableMessageEvent::ports() returns a HeapVector by value (either a copy, or a new empty one). Either way, these are all refcounted, so the lifetime of BudgetService shouldn't matter? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.h (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.h:21: class BudgetService final : public GarbageCollectedFinalized<BudgetService>, public ScriptWrappable { On 2016/08/18 10:23:26, harkness wrote: > On 2016/08/17 13:54:02, johnme wrote: > > Why does this need to be Finalized? > > m_service requires it. Some Mojo consumers reset their InterfacePtrs when the execution context is destroyed (e.g. USB::contextDestroyed), or the mojo connection fails (e.g. USB::on*ConnectionError, which also rejects the promises). Do either of those apply here? https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py:65: resources_dir = self._filesystem.join(test_dir, "resources") On 2016/08/18 10:23:26, harkness wrote: > On 2016/08/17 13:54:02, johnme wrote: > > What's the difference between this and imported_resourced_dir? Is there a > reason > > not to use that? > > This was actually a change I got from Peter. Originally he tried using > imported_resources_dir which didn't work, and then switched to this form. I > don't know the details of the apache config, so I'm not sure why the original > approach with imported_resources_dir didn't work. How does http/tests/security/powerfulFeatureRestrictions/resources/geolocation.html do it?
https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:16: let [budget_service, router] = mojo.modules; On 2016/08/18 13:04:31, johnme wrote: > On 2016/08/18 10:23:26, harkness wrote: > > On 2016/08/17 13:54:01, johnme wrote: > > > Nit: camelCase in JS > > > > budget_service is the name of the mojo service, so this is actually consistent > > with that, even if not with normal JS style. The other mock I'd looked at used > > this casing, do you think it's better to go JS style route or Mojo consistency > > route? > > geolocation-mock.js calls one of its loaded mojo services permissionStatus, so > consistency can go either way. Stick with JS style I guess? Sounds good, I'll change it. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:28: ScriptPromise BudgetService::getCost(ScriptState* scriptState, const AtomicString& /* actionType */) On 2016/08/18 13:04:31, johnme wrote: > On 2016/08/18 10:23:26, harkness wrote: > > On 2016/08/17 13:54:02, johnme wrote: > > > operationType? > > > > Actually, when it comes in to this function, it's action type and needs to be > > mapped to operation type. > > Oh, I was looking at https://beverloo.github.io/budget-api/ but I see that you > already called it actionType in BudgetService.idl. Presumably later you'll > rename it to match the spec in both places? Yup, that's definitely on the roadmap. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.cpp:80: budget[i] = freezeV8Object(toV8(chunk, scriptState.get()), scriptState->isolate()); On 2016/08/18 13:04:31, johnme wrote: > On 2016/08/18 10:23:26, harkness wrote: > > On 2016/08/17 13:54:01, johnme wrote: > > > BudgetState objects shouldn't need to be individually frozen, since all > their > > > attributes are readonly primitives (once you update BudgetChunk to match the > > > spec and stop being a dictionary). This might mean you no longer need to > deal > > > with v8 directly, which would be nice. > > > > I think I can get away without the extra freezeV8Object call, but I still > think > > I need the toV8, and that's where I needed the extra scope. > > > > The difficulty here is that all other instances of FrozenArray returning an > > object are returning a reference to a member object of the Blink class, so > they > > can rely on the class to control the lifetime. In my case, I need to > explicitly > > allocate a copy of the array on the v8 heap because after that method returns, > > the memory that was passed from the browser will be deallocated. > > It looks like ExtendableMessageEvent::ports() returns a HeapVector by value > (either a copy, or a new empty one). Either way, these are all refcounted, so > the lifetime of BudgetService shouldn't matter? I hadn't realized that BudgetState (formerly BudgetChunk) had been switched from a dictionary to an interface, so I updated for that. Once I did that, the BudgetState was a refcounted object, so I could put it in a HeapVector and trust to that to keep it alive. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.h (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.h:21: class BudgetService final : public GarbageCollectedFinalized<BudgetService>, public ScriptWrappable { On 2016/08/18 13:04:31, johnme wrote: > On 2016/08/18 10:23:26, harkness wrote: > > On 2016/08/17 13:54:02, johnme wrote: > > > Why does this need to be Finalized? > > > > m_service requires it. > > Some Mojo consumers reset their InterfacePtrs when the execution context is > destroyed (e.g. USB::contextDestroyed), or the mojo connection fails (e.g. > USB::on*ConnectionError, which also rejects the promises). Do either of those > apply here? It appears that mojo Ptrs can't be traced using the oilpan lifecycle tracing, so using that mechanism is out. When I try to compile without Finalized, I get an explicit compiler error that m_service requires finalization, which I'm guessing is because it's a mojo service. Looking at the InterfacePtr in mojo, it closes the message pipe on destruction, so I suspect that destructor invocation is being enforced by requiring Finalized. https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... File third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Tool... third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/servers/apache_http.py:65: resources_dir = self._filesystem.join(test_dir, "resources") On 2016/08/18 13:04:31, johnme wrote: > On 2016/08/18 10:23:26, harkness wrote: > > On 2016/08/17 13:54:02, johnme wrote: > > > What's the difference between this and imported_resourced_dir? Is there a > > reason > > > not to use that? > > > > This was actually a change I got from Peter. Originally he tried using > > imported_resources_dir which didn't work, and then switched to this form. I > > don't know the details of the apache config, so I'm not sure why the original > > approach with imported_resources_dir didn't work. > > How does > http/tests/security/powerfulFeatureRestrictions/resources/geolocation.html do > it? Ah, it looks like they added test_dir to make it accessible via the js-test-resources path. I'll update my test to us that path.
lgtm as long as you get someone with more mojo experience to check the BudgetService lifetime https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetService.h (right): https://codereview.chromium.org/2231873002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetService.h:21: class BudgetService final : public GarbageCollectedFinalized<BudgetService>, public ScriptWrappable { On 2016/08/22 08:57:47, harkness wrote: > On 2016/08/18 13:04:31, johnme wrote: > > On 2016/08/18 10:23:26, harkness wrote: > > > On 2016/08/17 13:54:02, johnme wrote: > > > > Why does this need to be Finalized? > > > > > > m_service requires it. > > > > Some Mojo consumers reset their InterfacePtrs when the execution context is > > destroyed (e.g. USB::contextDestroyed), or the mojo connection fails (e.g. > > USB::on*ConnectionError, which also rejects the promises). Do either of those > > apply here? > > It appears that mojo Ptrs can't be traced using the oilpan lifecycle tracing, so > using that mechanism is out. When I try to compile without Finalized, I get an > explicit compiler error that m_service requires finalization, which I'm guessing > is because it's a mojo service. Looking at the InterfacePtr in mojo, it closes > the message pipe on destruction, so I suspect that destructor invocation is > being enforced by requiring Finalized. Yes, my question is whether we should be cleaning it up earlier, rather than leaving it active in a potentially invalid state. I'm not an expert here, but I know that there are subtleties.
https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:9: const TEST_BUDGET_TIME = new Date().getTime(); One thing to consider: Rather than hard coding these values, you could add methods on the BudgetServiceMock class enabling the test to indicate which values they expect. For instance, you could consider having something like: ==================== class BudgetServiceMock { constructor(interfaceProvider) { // ... this.costs_ = new Map(); } // ... getCost(operationType) { return Promise.resolve({ cost: this.costs_.get(operationType) || 0 }); } setCostForTesting(operationType, cost) { this.costs_.set(operationType, cost); } } ==================== And then in your test: ==================== promise_test(function() { const silentPushCost = 42; return budgetServiceMock.then(mock => { assert_own_property(Navigator.prototype, 'budget'); mock.setCostForTesting("silent-push", silentPushCost); return navigator.budget.getCost("silent-push"); }).then(cost => { assert_equals(cost, silentPushCost); }); }, 'BudgetService mock for GetCost should return correct results.'); ==================== That completely localizes the knowledge, and removes the need for the constants. It does require some diligence to avoid depending on the execution order of the tests, so some sort of "clearOverridesForTesting" method might be advisable too. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:16: let [budgetService, router] = mojo.modules; nit: s/let/const/ https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:41: // Connect to the Mojo interface. nit: drop the comment. This doesn't actually "connect" to the interface, it registers an override to whatever service is being provided by default. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/success-query-path.html (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/success-query-path.html:22: }, 'BudgetService mock for GetCost should return correct results.'); nit: I would split this up to one test per function. For instance, for getCost() you'd want to test the behaviour of calling it with an unrecognized argument. It would also act as an additional guard against depending on test execution order. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:22: DCHECK(m_service); I don't think this DCHECK is valid— when getting the interface from a worker context, which is *not* on the main thread, a jump would occur prior to the service being bound. Remove it? https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:61: for (uint i = 0; i < expectations.size(); i++) nit: we usually use size_t for this, which is also the return type of WTFArray<>.size(). https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.h (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.h:36: // Callbacks from the BudgetService to the blink layer. nit: both can be private https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.h:37: void gotCost(ScriptPromiseResolver*, double) const; nit: name the double https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.cpp (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.cpp:9: BudgetState::BudgetState() This would leave m_budgetAt and m_time uninitialized, which could yield unexpected behaviour. Please initialize them. Also, why not just define both constructors out-of-line (thus in the .cc file)? https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.h (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:9: #include "core/dom/DOMTimeStamp.h" #include "platform/heap/GarbageCollected.h" https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:22: BudgetState(const double budgetAt, const DOMTimeStamp& time) nit: no need to mark scalar types as "const" (goes for both arguments) https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:36: private: nit: blank line above here https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.idl (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.idl:10: readonly attribute DOMTimeStamp time; nit: +2 spaces
https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:9: const TEST_BUDGET_TIME = new Date().getTime(); On 2016/08/22 17:57:55, Peter Beverloo wrote: > One thing to consider: > > Rather than hard coding these values, you could add methods on the > BudgetServiceMock class enabling the test to indicate which values they expect. > For instance, you could consider having something like: > > ==================== > class BudgetServiceMock { > constructor(interfaceProvider) { > // ... > > this.costs_ = new Map(); > } > > // ... > > getCost(operationType) { > return Promise.resolve({ cost: this.costs_.get(operationType) || 0 }); > } > > setCostForTesting(operationType, cost) { > this.costs_.set(operationType, cost); > } > } > ==================== > > And then in your test: > > ==================== > promise_test(function() { > const silentPushCost = 42; > > return budgetServiceMock.then(mock => { > assert_own_property(Navigator.prototype, 'budget'); > > mock.setCostForTesting("silent-push", silentPushCost); > > return navigator.budget.getCost("silent-push"); > }).then(cost => { > assert_equals(cost, silentPushCost); > }); > > }, 'BudgetService mock for GetCost should return correct results.'); > ==================== > > That completely localizes the knowledge, and removes the need for the constants. > > It does require some diligence to avoid depending on the execution order of the > tests, so some sort of "clearOverridesForTesting" method might be advisable too. I'll make this change in the next CL when I add in testing for Reserve. Right now I've got a bunch of changes pending on this change, so I want to clear that backlog. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:16: let [budgetService, router] = mojo.modules; On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: s/let/const/ Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/budget-service-mock.js:41: // Connect to the Mojo interface. On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: drop the comment. This doesn't actually "connect" to the interface, it > registers an override to whatever service is being provided by default. Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/success-query-path.html (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/success-query-path.html:22: }, 'BudgetService mock for GetCost should return correct results.'); On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: I would split this up to one test per function. For instance, for getCost() > you'd want to test the behaviour of calling it with an unrecognized argument. It > would also act as an additional guard against depending on test execution order. Done. We no longer have success-query-path, instead we have get-cost and get-budget. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:22: DCHECK(m_service); On 2016/08/22 17:57:55, Peter Beverloo wrote: > I don't think this DCHECK is valid— when getting the interface from a worker > context, which is *not* on the main thread, a jump would occur prior to the > service being bound. Remove it? Done. Added the check to getCost and getBudget instead. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:61: for (uint i = 0; i < expectations.size(); i++) On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: we usually use size_t for this, which is also the return type of > WTFArray<>.size(). Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.h (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.h:36: // Callbacks from the BudgetService to the blink layer. On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: both can be private Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.h:37: void gotCost(ScriptPromiseResolver*, double) const; On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: name the double Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.cpp (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.cpp:9: BudgetState::BudgetState() On 2016/08/22 17:57:55, Peter Beverloo wrote: > This would leave m_budgetAt and m_time uninitialized, which could yield > unexpected behaviour. Please initialize them. > > Also, why not just define both constructors out-of-line (thus in the .cc file)? Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.h (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:9: #include "core/dom/DOMTimeStamp.h" On 2016/08/22 17:57:55, Peter Beverloo wrote: > #include "platform/heap/GarbageCollected.h" Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:22: BudgetState(const double budgetAt, const DOMTimeStamp& time) On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: no need to mark scalar types as "const" (goes for both arguments) Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:36: private: On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: blank line above here Done. https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.idl (right): https://codereview.chromium.org/2231873002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.idl:10: readonly attribute DOMTimeStamp time; On 2016/08/22 17:57:55, Peter Beverloo wrote: > nit: +2 spaces Done.
peter@chromium.org changed reviewers: + haraken@chromium.org
+haraken, would you mind taking a look too? lgtm https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/get-budget.html (right): https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-budget.html:22: assert_equals(budget[0].time, TEST_BUDGET_TIME); nit: please consistently indent your code (either two or four spaces) https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-budget.html:24: }, 'BudgetService mock for GetBudget should return correct results.'); Here and elsewhere: you make it sound like you're testing the mock, but you're really testing the code path within Blink. In addition, this description doesn't match the test. https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/get-cost.html (right): https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-cost.html:20: assert_equals(cost, TEST_BUDGET_COST); dito. Also on line 29 https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-cost.html:27: return navigator.budget.getCost("push"); nit: I'd choose something that's obviously fake. What about "frobinator"? https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.h (right): https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:17: // This exposes the budget object on the Navigator partial interface. update comment
https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/get-budget.html (right): https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-budget.html:22: assert_equals(budget[0].time, TEST_BUDGET_TIME); On 2016/08/23 09:46:39, Peter Beverloo wrote: > nit: please consistently indent your code (either two or four spaces) Sorry about that, for some reason the auto-indenting isn't working well for Javascript in HTML. Fixed now. https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-budget.html:24: }, 'BudgetService mock for GetBudget should return correct results.'); On 2016/08/23 09:46:38, Peter Beverloo wrote: > Here and elsewhere: you make it sound like you're testing the mock, but you're > really testing the code path within Blink. In addition, this description doesn't > match the test. Done. https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/get-cost.html (right): https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-cost.html:20: assert_equals(cost, TEST_BUDGET_COST); On 2016/08/23 09:46:39, Peter Beverloo wrote: > dito. Also on line 29 Done. https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/get-cost.html:27: return navigator.budget.getCost("push"); On 2016/08/23 09:46:39, Peter Beverloo wrote: > nit: I'd choose something that's obviously fake. What about "frobinator"? Done. https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.h (right): https://codereview.chromium.org/2231873002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.h:17: // This exposes the budget object on the Navigator partial interface. On 2016/08/23 09:46:39, Peter Beverloo wrote: > update comment Done.
The CQ bit was checked by harkness@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...
LGTM https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:55: RefPtr<SecurityOrigin> origin(scriptState->getExecutionContext()->getSecurityOrigin()); scriptState->getExecutionContext() may return false if the script is being executed on a detached frame. You need a nullptr check.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:21: Platform::current()->interfaceProvider()->getInterface(mojo::GetProxy(&m_service)); Per discussion on this thread (https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/QpKZPmdWpYY/di...), it seems better to pass in LocalFrame& to the constructor and use a per-frame InterfaceProvider instead of the platform-wide InterfaceProvider.
https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:21: Platform::current()->interfaceProvider()->getInterface(mojo::GetProxy(&m_service)); On 2016/08/23 15:02:59, haraken wrote: > > Per discussion on this thread > (https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/QpKZPmdWpYY/di...), > it seems better to pass in LocalFrame& to the constructor and use a per-frame > InterfaceProvider instead of the platform-wide InterfaceProvider. This is going to be used in both worker (next CL) and document, so there may not be a frame. https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:55: RefPtr<SecurityOrigin> origin(scriptState->getExecutionContext()->getSecurityOrigin()); On 2016/08/23 11:24:12, haraken wrote: > > scriptState->getExecutionContext() may return false if the script is being > executed on a detached frame. You need a nullptr check. > Done.
https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetService.cpp (right): https://codereview.chromium.org/2231873002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetService.cpp:21: Platform::current()->interfaceProvider()->getInterface(mojo::GetProxy(&m_service)); On 2016/08/23 15:10:46, harkness wrote: > On 2016/08/23 15:02:59, haraken wrote: > > > > Per discussion on this thread > > > (https://groups.google.com/a/chromium.org/d/topic/chromium-mojo/QpKZPmdWpYY/di...), > > it seems better to pass in LocalFrame& to the constructor and use a per-frame > > InterfaceProvider instead of the platform-wide InterfaceProvider. > > This is going to be used in both worker (next CL) and document, so there may not > be a frame. Then the platform-wide InterfaceProvider makes sense.
The CQ bit was checked by harkness@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...
harkness@chromium.org changed reviewers: + tsepez@chromium.org
+tsepez Please review this and https://codereview.chromium.org/2265173002/ (adding you as reviewer there also) for Mojo sanity.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by harkness@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...
harkness@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng could you take a look at third_party/Webkit/public please?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry, I am at an offsite. I will review Wednesday PST.
https://codereview.chromium.org/2231873002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.idl (right): https://codereview.chromium.org/2231873002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.idl:8: interface BudgetState { [ RuntimeEnabled=Budget ] interface BudgetState { // ... }
https://codereview.chromium.org/2231873002/diff/180001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/BudgetState.idl (right): https://codereview.chromium.org/2231873002/diff/180001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/BudgetState.idl:8: interface BudgetState { On 2016/08/24 15:40:57, Peter Beverloo wrote: > [ > RuntimeEnabled=Budget > ] interface BudgetState { > // ... > } Done.
Mojom itself LGTM.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org, peter@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2231873002/#ps220001 (title: "Add a connection error handler to BudgetService")
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 peter@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...)
aelias@chromium.org changed reviewers: + aelias@chromium.org
WebKit/public lgtm
The CQ bit was checked by peter@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2231873002/diff/220001/third_party/WebKit/pub... File third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom (right): https://codereview.chromium.org/2231873002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:10: SILENT_PUSH FWIW, an enum with one value looks kinda funny. https://codereview.chromium.org/2231873002/diff/220001/third_party/WebKit/pub... third_party/WebKit/public/platform/modules/budget_service/budget_service.mojom:21: double time; Can we add a TODO here to transition to base::Time (https://cs.chromium.org/chromium/src/mojo/common/common_custom_types.mojom?rc...) at some point?
Message was sent while issue was closed.
Description was changed from ========== Added budget_service.mojom This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. This also adds a mock and a layout test to check the mojo service. BUG=617971 ========== to ========== Added budget_service.mojom This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. This also adds a mock and a layout test to check the mojo service. BUG=617971 ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Added budget_service.mojom This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. This also adds a mock and a layout test to check the mojo service. BUG=617971 ========== to ========== Added budget_service.mojom This isn't implemented yet, but will be provide the Mojo service that will provide blink an access point to the BackgroundBudgetService. This also adds a mock and a layout test to check the mojo service. BUG=617971 Committed: https://crrev.com/7e00f5e7b1fdedd2a32ef8321bb13e2f0726bd83 Cr-Commit-Position: refs/heads/master@{#414265} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/7e00f5e7b1fdedd2a32ef8321bb13e2f0726bd83 Cr-Commit-Position: refs/heads/master@{#414265} |
