Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2110833002:
Basic framework for Budget API. (Closed)
DescriptionBasic framework for Budget API.
This implements the basic framework for the Budget API in blink. Only the
getCost and getBudget methods are defined, and a LayoutTest is specified
which checks that the interfaces exist.
BUG=617971
Committed: https://crrev.com/868da920b60dc73aafa236c288f4cf93a6610a4c
Cr-Commit-Position: refs/heads/master@{#410378}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Added getDetailedBudget and trace. #Patch Set 3 : Added BudgetChunk.idl #
Total comments: 28
Patch Set 4 : Integrated code review comments. #
Total comments: 12
Patch Set 5 : Updated to (almost) match the spec #
Total comments: 32
Patch Set 6 : Removed Navigator member and exception, other nits #
Total comments: 22
Patch Set 7 : Added interface tests and resolved nits #
Total comments: 4
Patch Set 8 : Removed Exposed lines and fixed build error. #Messages
Total messages: 35 (17 generated)
harkness@chromium.org changed reviewers: + peter@chromium.org
This is the very basic interface we talked about, which can be extended as I add in more functionality and testing.
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2110833002/diff/1/third_party/WebKit/Source/m... File third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/1/third_party/WebKit/Source/m... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:29: public: You need to add DECLARE_TRACE/DEFINE_TRACE. I guess that not having the tracing method will lead to a compile error, but didn't it?
It probably would have complained if I'd run a full build, but my local build didn't. I've updated that and also added in the interface for getBudgetDetails which peter@ asked for in person.
Thanks, cool to see! :-)
A few higher-level comments:
- The API is called "Budget API", yet the directory is named
"budget_service". That seems odd. Let's just name the directory "budget"?
- Just to reiterate: we'll need an Intent to Implement before this can land.
- Please add an OWNERS file in the directory listing me, until we can add
you as well.
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/http/tests/budget_service/interfaces.html
(right):
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/budget_service/interfaces.html:14:
assert_own_property(Navigator.prototype, 'getBudgetDetails');
nit: You could reject the promises for now rather than leaving them hanging,
allowing you to test a wee bit more of the behaviour.
=====
return ScriptPromise::rejectWithDOMException(scriptState,
DOMException::create(NotSupportedError, "Not yet implemented."));
=====
(Allows you to remove the separate Resolver/Promise/return Promise.)
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/budget_service/BudgetChunk.idl (right):
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/BudgetChunk.idl:8: double
budget_amount;
Let's call this `budget` or `amount`.
There is a *strong* preference for having short names. The fact that this will
be returned from a method called get*Budget() already implies that it's about
budget, so I'd opt for `amount`.
In addition, things in JavaScript land are named using camelCase, not
hacker_style.
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/BudgetChunk.idl:9: double
expiration_timestamp;
`expiration`?
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp
(right):
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:9: #include
"bindings/core/v8/ScriptState.h"
unused
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:10:
#include "core/dom/ExceptionCode.h"
unused
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:21: //
Static
nit: // static (no capital)
dito on lines 27, 41, 53 and 65
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:44: const
AtomicString& actionType)
nit: here (and with the other method definitions): place all parameters on the
same line.
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h (right):
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:20: // This
is the entry point into the browser for the BudgetAPI, which is
nit: BudgetAPI -> Budget API
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:21: //
designed to give Service Workers the ability to perform background
Service Workers -> origins
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:23: class
MODULES_EXPORT NavigatorBudget final
Why do you need MODULES_EXPORT here?
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:24: : public
GarbageCollectedFinalized<NavigatorBudget>
Why do you need a finalizer here? This can just be GarbageCollected<> and you
can remove the destructor.
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/budget_service/NavigatorBudget.idl
(right):
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.idl:10: [
nit: blank line before this
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/budget_service/NavigatorBudget.idl:15:
[CallWith=ScriptState, RaisesException] Promise<FrozenArray<BudgetChunk>>
getBudgetDetails();
Why do you annotate getBudget() and getBudgetDetails() with RaisesException when
they don't actually raise an exception? :)
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:37: BudgetService
status=experimental
Let's keep this as status=test until it's able to do something sensible.
https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget_service/interfaces.html (right): https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget_service/interfaces.html:14: assert_own_property(Navigator.prototype, 'getBudgetDetails'); On 2016/07/01 13:21:59, Peter Beverloo wrote: > nit: You could reject the promises for now rather than leaving them hanging, > allowing you to test a wee bit more of the behaviour. > > ===== > return ScriptPromise::rejectWithDOMException(scriptState, > DOMException::create(NotSupportedError, "Not yet implemented.")); > ===== > > (Allows you to remove the separate Resolver/Promise/return Promise.) Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget_service/BudgetChunk.idl (right): https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/BudgetChunk.idl:8: double budget_amount; On 2016/07/01 13:21:59, Peter Beverloo wrote: > Let's call this `budget` or `amount`. > > There is a *strong* preference for having short names. The fact that this will > be returned from a method called get*Budget() already implies that it's about > budget, so I'd opt for `amount`. > > In addition, things in JavaScript land are named using camelCase, not > hacker_style. Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/BudgetChunk.idl:9: double expiration_timestamp; On 2016/07/01 13:21:59, Peter Beverloo wrote: > `expiration`? Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp (right): https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:9: #include "bindings/core/v8/ScriptState.h" On 2016/07/01 13:21:59, Peter Beverloo wrote: > unused It's used now. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:10: #include "core/dom/ExceptionCode.h" On 2016/07/01 13:21:59, Peter Beverloo wrote: > unused It's used now. :) https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:21: // Static On 2016/07/01 13:21:59, Peter Beverloo wrote: > nit: // static (no capital) > > dito on lines 27, 41, 53 and 65 Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.cpp:44: const AtomicString& actionType) On 2016/07/01 13:21:59, Peter Beverloo wrote: > nit: here (and with the other method definitions): place all parameters on the > same line. Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:20: // This is the entry point into the browser for the BudgetAPI, which is On 2016/07/01 13:21:59, Peter Beverloo wrote: > nit: BudgetAPI -> Budget API Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:21: // designed to give Service Workers the ability to perform background On 2016/07/01 13:21:59, Peter Beverloo wrote: > Service Workers -> origins Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:23: class MODULES_EXPORT NavigatorBudget final On 2016/07/01 13:21:59, Peter Beverloo wrote: > Why do you need MODULES_EXPORT here? Removed. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.h:24: : public GarbageCollectedFinalized<NavigatorBudget> On 2016/07/01 13:21:59, Peter Beverloo wrote: > Why do you need a finalizer here? This can just be GarbageCollected<> and you > can remove the destructor. Updated it. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget_service/NavigatorBudget.idl (right): https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.idl:10: [ On 2016/07/01 13:21:59, Peter Beverloo wrote: > nit: blank line before this Done. https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget_service/NavigatorBudget.idl:15: [CallWith=ScriptState, RaisesException] Promise<FrozenArray<BudgetChunk>> getBudgetDetails(); On 2016/07/01 13:21:59, Peter Beverloo wrote: > Why do you annotate getBudget() and getBudgetDetails() with RaisesException when > they don't actually raise an exception? :) I assumed that they would eventually, an in fact they do now! https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/2110833002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:37: BudgetService status=experimental On 2016/07/01 13:21:59, Peter Beverloo wrote: > Let's keep this as status=test until it's able to do something sensible. Done.
This largely seems fine as-is, but we probably want to reflect the decisions we made about the API yesterday in the initial CL? https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:7: <script src="../serviceworker/resources/test-helpers.js"></script> What are you using from this file? https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:11: test(function() { Please use promise_test. Right now, your test will pass even if the promii are settled with something other than the NotSupportedError rejection because the end of the test's control flow will be reached before the promii settle, which is done through a microtask. https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetChunk.idl (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetChunk.idl:5: // TODO(harkness) Add reference to W3C standard doc. nit: s/W3C standard doc/specification/ (dito elsewhere) Specs don't necessarily live at the W3C, and for those which do we don't call them standards until they reach recommendation level, which is still very far off :) https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp:64: // ContextLifecycleObserver::trace(visitor); remove https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:22: // operations on the user's behalf. micro nit: wrapping in this comment looks arbitrary? https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:37: DECLARE_VIRTUAL_TRACE(); DECLARE_TRACE? I don't think we need the virtualness.
https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:7: <script src="../serviceworker/resources/test-helpers.js"></script> On 2016/07/22 12:27:01, Peter Beverloo wrote: > What are you using from this file? Removed. https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:11: test(function() { On 2016/07/22 12:27:01, Peter Beverloo wrote: > Please use promise_test. > > Right now, your test will pass even if the promii are settled with something > other than the NotSupportedError rejection because the end of the test's control > flow will be reached before the promii settle, which is done through a > microtask. Done. https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetChunk.idl (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetChunk.idl:5: // TODO(harkness) Add reference to W3C standard doc. On 2016/07/22 12:27:01, Peter Beverloo wrote: > nit: s/W3C standard doc/specification/ (dito elsewhere) > > Specs don't necessarily live at the W3C, and for those which do we don't call > them standards until they reach recommendation level, which is still very far > off :) Done. https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp:64: // ContextLifecycleObserver::trace(visitor); On 2016/07/22 12:27:01, Peter Beverloo wrote: > remove Done. https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:22: // operations on the user's behalf. On 2016/07/22 12:27:01, Peter Beverloo wrote: > micro nit: wrapping in this comment looks arbitrary? Done. https://codereview.chromium.org/2110833002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:37: DECLARE_VIRTUAL_TRACE(); On 2016/07/22 12:27:01, Peter Beverloo wrote: > DECLARE_TRACE? I don't think we need the virtualness. Done.
https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:29: }, 'NavigatorBudget should be exposed and have the expected interface in a Document.'); nit: fix indentation in this file https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/Budget.cpp (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.cpp:30: ScriptPromise Budget::getBudget(ScriptState* state, ExceptionState& exception) nit: drop the RaisesException annotation in the IDL file until we need it? We can reject based on |state|, that doesn't need |exception|. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/Budget.h (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.h:23: public ScriptWrappable { nit: I wouldn't wrap these three lines. Even in Chromium style they'd fit on a single line. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.h:29: return new Budget(navigator); nit: drop the |navigator| argument, we're not using it. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.h:33: static ScriptPromise getBudget(ScriptState*, ExceptionState&); nit: These methods shouldn't be static. NavigatorBudget returns an instance, so we can just operate on that. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/Budget.idl (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:5: // https://beverloo.github.io/budget-api/ # https://beverloo.github.io/budget-api/#budget-service-interface https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:5: // https://beverloo.github.io/budget-api/ nit: Add a TODO for linking to a WICG spec when it exists? https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:12: Exposed=(Window, ServiceWorker), The spec says [Exposed=(Window,Worker)], you use Worker -> ServiceWorker. Any particular reason? https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:14: ] interface Budget { Something to consider: The spec calls this BudgetService, you call this Budget. One should change. Preferences? https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetChunk.idl (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetChunk.idl:5: // https://beverloo.github.io/budget-api/ # https://beverloo.github.io/budget-api/#budget-state-interface https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp:34: // this navigator, create one now and associate it. nit: the comment on line 33/34 doesn't really add much value. I don't mind too much either, up to you. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:17: public GarbageCollectedFinalized<NavigatorBudget>, nit: drop the "Finalized" and remove the destructor https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:35: Member<Navigator> m_navigator; Why do you store |m_navigator| here? https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.idl (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:5: // https://beverloo.github.io/budget-api/ # https://beverloo.github.io/budget-api/#navigator-budget-interface https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:8: Exposed=(Window, ServiceWorker), dito re: Worker / ServiceWorker exposure https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:10: ] partial interface Navigator { Add a TODO for matching the spec?
https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:29: }, 'NavigatorBudget should be exposed and have the expected interface in a Document.'); On 2016/08/03 13:25:54, Peter Beverloo wrote: > nit: fix indentation in this file Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/Budget.cpp (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.cpp:30: ScriptPromise Budget::getBudget(ScriptState* state, ExceptionState& exception) On 2016/08/03 13:25:54, Peter Beverloo wrote: > nit: drop the RaisesException annotation in the IDL file until we need it? We > can reject based on |state|, that doesn't need |exception|. Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/Budget.h (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.h:23: public ScriptWrappable { On 2016/08/03 13:25:54, Peter Beverloo wrote: > nit: I wouldn't wrap these three lines. Even in Chromium style they'd fit on a > single line. Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.h:29: return new Budget(navigator); On 2016/08/03 13:25:54, Peter Beverloo wrote: > nit: drop the |navigator| argument, we're not using it. Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.h:33: static ScriptPromise getBudget(ScriptState*, ExceptionState&); On 2016/08/03 13:25:54, Peter Beverloo wrote: > nit: These methods shouldn't be static. NavigatorBudget returns an instance, so > we can just operate on that. Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/Budget.idl (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:5: // https://beverloo.github.io/budget-api/ On 2016/08/03 13:25:54, Peter Beverloo wrote: > # https://beverloo.github.io/budget-api/#budget-service-interface Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:5: // https://beverloo.github.io/budget-api/ On 2016/08/03 13:25:54, Peter Beverloo wrote: > nit: Add a TODO for linking to a WICG spec when it exists? Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:12: Exposed=(Window, ServiceWorker), On 2016/08/03 13:25:55, Peter Beverloo wrote: > The spec says [Exposed=(Window,Worker)], you use Worker -> ServiceWorker. Any > particular reason? This was here from an earlier version, and I wasn't sure about Worker or ServiceWorker. Changed now. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/Budget.idl:14: ] interface Budget { On 2016/08/03 13:25:55, Peter Beverloo wrote: > Something to consider: The spec calls this BudgetService, you call this Budget. > One should change. Preferences? When we were talking the other day, I suggested BudgetService, but it seemed that consensus was for just Budget. Have you gotten any feedback about this naming on the spec? For this CL, I'm going to leave it as Budget. I can change it if we decide to. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/BudgetChunk.idl (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/BudgetChunk.idl:5: // https://beverloo.github.io/budget-api/ On 2016/08/03 13:25:55, Peter Beverloo wrote: > # https://beverloo.github.io/budget-api/#budget-state-interface Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp:34: // this navigator, create one now and associate it. On 2016/08/03 13:25:55, Peter Beverloo wrote: > nit: the comment on line 33/34 doesn't really add much value. I don't mind too > much either, up to you. It's not a huge value, but I do think it makes the code more readable for a new person. Shortened it though. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:17: public GarbageCollectedFinalized<NavigatorBudget>, On 2016/08/03 13:25:55, Peter Beverloo wrote: > nit: drop the "Finalized" and remove the destructor Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:35: Member<Navigator> m_navigator; On 2016/08/03 13:25:55, Peter Beverloo wrote: > Why do you store |m_navigator| here? Discussed in person, removing it. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/budget/NavigatorBudget.idl (right): https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:5: // https://beverloo.github.io/budget-api/ On 2016/08/03 13:25:55, Peter Beverloo wrote: > # https://beverloo.github.io/budget-api/#navigator-budget-interface Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:8: Exposed=(Window, ServiceWorker), On 2016/08/03 13:25:55, Peter Beverloo wrote: > dito re: Worker / ServiceWorker exposure Done. https://codereview.chromium.org/2110833002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:10: ] partial interface Navigator { On 2016/08/03 13:25:55, Peter Beverloo wrote: > Add a TODO for matching the spec? 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...
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_...)
lgtm This is a good base to start iterating upon, so let's do it. haraken@: Would you mind taking a look from the perspective of adding a new module? https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:14: nit: consider testing assert_equals(navigator.budget, navigator.budget); https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.cpp (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:12: #include "core/frame/Navigator.h" nit: unused https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:30: DEFINE_TRACE(Budget) nit: Since it's empty, you could drop this and swap out DECLARE_TRACE in Budget.h with DEFINE_INLINE_TRACE() { } https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.h (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.h:14: class Navigator; nit: ExceptionState and Navigator are unused https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.h:21: class Budget final : public GarbageCollected<Budget> , public ScriptWrappable { nit: no space before the comma https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.h:36: explicit Budget(); nit: drop "explicit" https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.idl (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.idl:13: Exposed=(Window, Worker), nit: consider removing this until you expose it from WorkerNavigator as well https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:31: explicit NavigatorBudget(); nit: drop "explicit"
Oh, also fix the webexposed tests of course, which we spoke about yesterday :-).
Implementation-wise LGTM https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.cpp (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:18: ScriptPromise Budget::getCost(ScriptState* state, const AtomicString& actionType) state => scriptState (we normally use scriptState to distinguish it from other states) https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:24: ScriptPromise Budget::getBudget(ScriptState* state) state => scriptState https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp:38: m_budget = Budget::create(); Is there any benefit in instantiating m_budget lazily?
https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/budget/interfaces.html:14: On 2016/08/05 14:33:19, Peter Beverloo wrote: > nit: consider testing > > assert_equals(navigator.budget, navigator.budget); Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.cpp (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:12: #include "core/frame/Navigator.h" On 2016/08/05 14:33:19, Peter Beverloo wrote: > nit: unused Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:18: ScriptPromise Budget::getCost(ScriptState* state, const AtomicString& actionType) On 2016/08/08 01:48:13, haraken wrote: > > state => scriptState (we normally use scriptState to distinguish it from other > states) Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:24: ScriptPromise Budget::getBudget(ScriptState* state) On 2016/08/08 01:48:13, haraken wrote: > > state => scriptState Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.cpp:30: DEFINE_TRACE(Budget) On 2016/08/05 14:33:19, Peter Beverloo wrote: > nit: Since it's empty, you could drop this and swap out DECLARE_TRACE in > Budget.h with > > DEFINE_INLINE_TRACE() { } Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.h (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.h:14: class Navigator; On 2016/08/05 14:33:19, Peter Beverloo wrote: > nit: ExceptionState and Navigator are unused Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.h:21: class Budget final : public GarbageCollected<Budget> , public ScriptWrappable { On 2016/08/05 14:33:19, Peter Beverloo wrote: > nit: no space before the comma Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.h:36: explicit Budget(); On 2016/08/05 14:33:19, Peter Beverloo wrote: > nit: drop "explicit" Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.idl (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.idl:13: Exposed=(Window, Worker), On 2016/08/05 14:33:20, Peter Beverloo wrote: > nit: consider removing this until you expose it from WorkerNavigator as well Done. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/NavigatorBudget.cpp:38: m_budget = Budget::create(); On 2016/08/08 01:48:13, haraken wrote: > > Is there any benefit in instantiating m_budget lazily? The Budget object is going to be a fairly heavy-weight object, and we don't anticipate most webapps will use it at all, so avoiding creating one should be a performance win. https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/NavigatorBudget.h (right): https://codereview.chromium.org/2110833002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/NavigatorBudget.h:31: explicit NavigatorBudget(); On 2016/08/05 14:33:20, Peter Beverloo wrote: > nit: drop "explicit" 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.idl (right): https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.idl:13: Exposed=(Window), nit: drop https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/NavigatorBudget.idl (right): https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:9: Exposed=(Window, Worker), nit: drop
The CQ bit was checked by harkness@chromium.org to run a CQ dry run
https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/Budget.idl (right): https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/Budget.idl:13: Exposed=(Window), On 2016/08/08 12:45:01, Peter Beverloo wrote: > nit: drop Done. https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/modules/budget/NavigatorBudget.idl (right): https://codereview.chromium.org/2110833002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/modules/budget/NavigatorBudget.idl:9: Exposed=(Window, Worker), On 2016/08/08 12:45:01, Peter Beverloo wrote: > nit: drop 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: This issue passed the CQ dry run.
The CQ bit was checked by harkness@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2110833002/#ps140001 (title: "Removed Exposed lines and fixed build error.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Basic framework for Budget API. This implements the basic framework for the Budget API in blink. Only the getCost and getBudget methods are defined, and a LayoutTest is specified which checks that the interfaces exist. BUG=617971 ========== to ========== Basic framework for Budget API. This implements the basic framework for the Budget API in blink. Only the getCost and getBudget methods are defined, and a LayoutTest is specified which checks that the interfaces exist. BUG=617971 Committed: https://crrev.com/868da920b60dc73aafa236c288f4cf93a6610a4c Cr-Commit-Position: refs/heads/master@{#410378} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/868da920b60dc73aafa236c288f4cf93a6610a4c Cr-Commit-Position: refs/heads/master@{#410378} |
