Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(572)

Issue 14773025: Create ResourceProgressEvent, expose as Chromium API (Closed)

Created:
7 years, 7 months ago by dmichael (off chromium)
Modified:
7 years, 6 months ago
CC:
blink-reviews, jamesr, eae+blinkwatch, abarth_chromum.org
Base URL:
https://chromium.googlesource.com/chromium/blink@master
Visibility:
Public.

Description

Create ResourceProgressEvent, expose as Chromium API This new event is needed so that we can dispatch NaCl load progress events from within the renderer. Currently, the NaCl plugin creates a ProgressEvent and attaches a "url" property using PPAPI synchronous scripting. Existing NaCl apps (Unity ones and possibly others) rely on the "url" property. The "url" property is useful for NaCl apps that use shared libraries, which will download several resources (each shared object and the .nexe), and the NaCl runtime reports progress for each resource separately. See https://codereview.chromium.org/14588009/ for the Chromium side of this change. BUG=239656 NOTRY=true Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=151533

Patch Set 1 : #

Total comments: 2

Patch Set 2 : Introduce ResourceProgressEvent #

Patch Set 3 : pre-review cleanup #

Total comments: 8

Patch Set 4 : Changes based on review feedback #

Patch Set 5 : (merge) #

Total comments: 4

Patch Set 6 : Move stuff behind WEBKIT_IMPLEMENTATION #

Patch Set 7 : fix merge #

Patch Set 8 : Oops, m_private is WebPrivatePtr now #

Patch Set 9 : merge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+102 lines, -115 lines) Patch
M Source/WebKit/chromium/WebKit.gyp View 1 2 3 4 5 6 7 8 2 chunks +4 lines, -0 lines 0 comments Download
A + Source/WebKit/chromium/public/WebDOMProgressEvent.h View 1 2 3 4 1 chunk +7 lines, -8 lines 0 comments Download
A + Source/WebKit/chromium/public/WebDOMResourceProgressEvent.h View 1 2 1 chunk +7 lines, -10 lines 0 comments Download
A + Source/WebKit/chromium/src/WebDOMProgressEvent.cpp View 1 2 3 4 5 6 7 1 chunk +12 lines, -13 lines 0 comments Download
A + Source/WebKit/chromium/src/WebDOMResourceProgressEvent.cpp View 1 2 3 1 chunk +9 lines, -18 lines 0 comments Download
M Source/core/core.gypi View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/dom/EventNames.in View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
A + Source/core/dom/ResourceProgressEvent.h View 1 2 3 4 1 chunk +32 lines, -35 lines 0 comments Download
A + Source/core/dom/ResourceProgressEvent.cpp View 1 2 3 4 5 6 7 8 2 chunks +15 lines, -16 lines 0 comments Download
A + Source/core/dom/ResourceProgressEvent.idl View 1 2 3 4 5 6 2 chunks +12 lines, -15 lines 0 comments Download

Messages

Total messages: 31 (0 generated)
dmichael (off chromium)
7 years, 7 months ago (2013-05-13 17:34:30 UTC) #1
darin (slow to review)
https://codereview.chromium.org/14773025/diff/1001/Source/WebKit/chromium/public/WebDOMProgressEvent.h File Source/WebKit/chromium/public/WebDOMProgressEvent.h (right): https://codereview.chromium.org/14773025/diff/1001/Source/WebKit/chromium/public/WebDOMProgressEvent.h#newcode49 Source/WebKit/chromium/public/WebDOMProgressEvent.h:49: WEBKIT_EXPORT v8::Handle<v8::Value> toV8Value(); I'm a bit worried about leaking ...
7 years, 7 months ago (2013-05-14 20:14:28 UTC) #2
abarth-chromium
https://codereview.chromium.org/14773025/diff/1001/Source/WebKit/chromium/public/WebDOMProgressEvent.h File Source/WebKit/chromium/public/WebDOMProgressEvent.h (right): https://codereview.chromium.org/14773025/diff/1001/Source/WebKit/chromium/public/WebDOMProgressEvent.h#newcode49 Source/WebKit/chromium/public/WebDOMProgressEvent.h:49: WEBKIT_EXPORT v8::Handle<v8::Value> toV8Value(); Having toV8Value here is no good. ...
7 years, 7 months ago (2013-05-14 20:17:50 UTC) #3
abarth-chromium
> toV8Value is required because when loading NaCl resources, > the NaCl plugin provides a ...
7 years, 7 months ago (2013-05-14 20:18:45 UTC) #4
dmichael (off chromium)
On 2013/05/14 20:17:50, abarth wrote: > https://codereview.chromium.org/14773025/diff/1001/Source/WebKit/chromium/public/WebDOMProgressEvent.h > File Source/WebKit/chromium/public/WebDOMProgressEvent.h (right): > > https://codereview.chromium.org/14773025/diff/1001/Source/WebKit/chromium/public/WebDOMProgressEvent.h#newcode49 > ...
7 years, 7 months ago (2013-05-14 20:20:03 UTC) #5
dmichael (off chromium)
On 2013/05/14 20:18:45, abarth wrote: > > toV8Value is required because when loading NaCl resources, ...
7 years, 7 months ago (2013-05-14 20:23:08 UTC) #6
abarth-chromium
Can we just drop the URL property? If we're using ProgressEvents here, we should use ...
7 years, 7 months ago (2013-05-14 20:25:47 UTC) #7
dmichael (off chromium)
On 2013/05/14 20:25:47, abarth wrote: > Can we just drop the URL property? If we're ...
7 years, 7 months ago (2013-05-14 20:40:14 UTC) #8
dmichael (off chromium)
I've found that a number of apps do expect to find the "url" property on ...
7 years, 7 months ago (2013-05-15 21:41:10 UTC) #9
dmichael (off chromium)
I think this is ready for another look. Thanks for your advice so far.
7 years, 7 months ago (2013-05-17 18:36:13 UTC) #10
abarth-chromium
Definitely looks on the right track. My comments below are mostly about documenting this class ...
7 years, 7 months ago (2013-05-17 19:06:12 UTC) #11
dmichael (off chromium)
PTAL https://codereview.chromium.org/14773025/diff/16001/Source/WebKit/chromium/src/WebDOMResourceProgressEvent.cpp File Source/WebKit/chromium/src/WebDOMResourceProgressEvent.cpp (right): https://codereview.chromium.org/14773025/diff/16001/Source/WebKit/chromium/src/WebDOMResourceProgressEvent.cpp#newcode44 Source/WebKit/chromium/src/WebDOMResourceProgressEvent.cpp:44: assign(ResourceProgressEvent::create(type, lengthIsComputable, loaded, total, url).leakRef()); On 2013/05/17 19:06:12, ...
7 years, 7 months ago (2013-05-20 21:11:24 UTC) #12
abarth-chromium
LGTM https://codereview.chromium.org/14773025/diff/31001/Source/WebKit/chromium/src/WebDOMEvent.cpp File Source/WebKit/chromium/src/WebDOMEvent.cpp (right): https://codereview.chromium.org/14773025/diff/31001/Source/WebKit/chromium/src/WebDOMEvent.cpp#newcode63 Source/WebKit/chromium/src/WebDOMEvent.cpp:63: m_private->deref(); Ouch. We should remove this function. It's ...
7 years, 7 months ago (2013-05-20 21:16:32 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/47002
7 years, 7 months ago (2013-05-23 16:45:50 UTC) #14
dmichael (off chromium)
Thanks again for the review. I'll land this now that the WebDOMEvent cleanup is done. ...
7 years, 7 months ago (2013-05-23 16:47:00 UTC) #15
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 7 months ago (2013-05-23 17:06:47 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/62001
7 years, 7 months ago (2013-05-23 17:16:13 UTC) #17
abarth-chromium
Thanks for taking the time to clean up WebDOMEvent.
7 years, 7 months ago (2013-05-23 17:49:25 UTC) #18
commit-bot: I haz the power
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=9672
7 years, 7 months ago (2013-05-23 19:18:34 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/62001
7 years, 7 months ago (2013-05-23 19:35:43 UTC) #20
commit-bot: I haz the power
Retried try job too often on linux_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_layout_rel&number=9702
7 years, 7 months ago (2013-05-23 21:29:09 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/62001
7 years, 7 months ago (2013-05-23 21:43:07 UTC) #22
commit-bot: I haz the power
Retried try job too often on mac_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_layout_rel&number=7110
7 years, 7 months ago (2013-05-23 23:10:15 UTC) #23
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/87001
7 years, 6 months ago (2013-05-30 16:33:27 UTC) #24
commit-bot: I haz the power
Retried try job too often on win_layout_rel for step(s) webkit_unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout_rel&number=7808
7 years, 6 months ago (2013-05-30 17:37:21 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/87001
7 years, 6 months ago (2013-05-30 17:56:05 UTC) #26
commit-bot: I haz the power
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout_rel&number=7842
7 years, 6 months ago (2013-05-30 21:21:36 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/87001
7 years, 6 months ago (2013-05-30 21:58:39 UTC) #28
commit-bot: I haz the power
Retried try job too often on win_layout_rel for step(s) webkit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_layout_rel&number=7891
7 years, 6 months ago (2013-05-31 00:59:43 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmichael@chromium.org/14773025/87001
7 years, 6 months ago (2013-05-31 08:22:45 UTC) #30
commit-bot: I haz the power
7 years, 6 months ago (2013-05-31 08:23:40 UTC) #31
Message was sent while issue was closed.
Change committed as 151533

Powered by Google App Engine
This is Rietveld 408576698