Bug 63376 - [DOM] Core part patch for supporting battery status event
: [DOM] Core part patch for supporting battery status event
Status: RESOLVED INVALID
Product: WebKit
Classification: Unclassified
Component: HTML DOM
: 528+ (Nightly build)
: All All
: P3 Normal
Assigned To: Nobody
http://dev.w3.org/2009/dap/system-inf...
:
Depends on:
Blocks: BatteryStatusAPI
  Show dependency treegraph
 
Reported: 2011-06-24 21:53 PDT by Kihong Kwon
Modified: 2012-04-05 00:15 PDT (History)
8 users (show)

See Also:


Attachments
Add core patch files for the battery status event (11.58 KB, patch)
2011-06-24 22:26 PDT, Kihong Kwon
gyuyoung.kim: commit‑queue-
Details | Formatted Diff | Diff
Add core patch files for the battery status event. (10.58 KB, patch)
2011-06-24 22:57 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Add core patch files for the battery status event (10.08 KB, patch)
2011-06-26 18:12 PDT, Kihong Kwon
leandro: review-
leandro: commit‑queue-
Details | Formatted Diff | Diff
Modify description in the ChangeLog (10.25 KB, patch)
2011-06-27 18:36 PDT, Kihong Kwon
leandro: review-
leandro: commit‑queue-
Details | Formatted Diff | Diff
Modify ChangeLog's comments (10.16 KB, patch)
2011-07-01 02:15 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Modify comment of ChangLog (10.12 KB, patch)
2011-07-01 03:49 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff
Add events for the revised spec. (11.80 KB, patch)
2011-07-06 23:43 PDT, Kihong Kwon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kihong Kwon 2011-06-24 21:53:59 PDT
This patch is a core part patch for battery status event(related bug 62698)
have to be patched after bug 62698
Comment 1 Kihong Kwon 2011-06-24 22:26:30 PDT
Created attachment 98579 [details]
Add core patch files for the battery status event
Comment 2 Gyuyoung Kim 2011-06-24 22:52:22 PDT
Comment on attachment 98579 [details]
Add core patch files for the battery status event

Attachment 98579 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/8934704
Comment 3 Kihong Kwon 2011-06-24 22:57:27 PDT
Created attachment 98581 [details]
Add core patch files for the battery status event.
Comment 4 Kihong Kwon 2011-06-26 18:12:00 PDT
Created attachment 98651 [details]
Add core patch files for the battery status event
Comment 5 Leandro Pereira 2011-06-27 10:08:39 PDT
Comment on attachment 98651 [details]
Add core patch files for the battery status event

View in context: https://bugs.webkit.org/attachment.cgi?id=98651&action=review

Looks OK, but ChangeLog entry needs work.

> Source/WebCore/ChangeLog:6
> +        [DOM] Core part patch for supporting battery status event
> +        https://bugs.webkit.org/show_bug.cgi?id=63376

"Add JavaScript wrappers for battery status events." would read better IMO. Also, things in brackets are usually used for port names. Since this is isn't port-specific, you can remove it.

> Source/WebCore/ChangeLog:14
> +        Add Core part modification for battery status event
> +        - JSEventCustom.cpp, V8EventCustom.cpp
> +          binding event between JavaSctipt and WebCore
> +        - dom/Document.cpp Event.cpp Event.h Eventnames.h
> +          Define and create event.
> +        - Page.h Page.cpp, DOMWindow.h DOMWindow.cpp
> +          Add/Remove event listener and init event handler to pageClient

These should go below, right after each file name.

> Source/WebCore/ChangeLog:18
> +        no new tests.
> +        This event implementation is not finished with this patch.
> +        After finishing inplemetation, It will be added.

Begin sentences with a capital letter. Better yet, rephrase this to say something like "Test cases will be added as soon as the whole battery status feature is implemented.".
Comment 6 Kihong Kwon 2011-06-27 18:36:27 PDT
Created attachment 98840 [details]
Modify description in the ChangeLog
Comment 7 Kihong Kwon 2011-06-27 18:37:29 PDT
Thank you for your comment.
I modified my ChangeLog.

(In reply to comment #5)
> (From update of attachment 98651 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=98651&action=review
> 
> Looks OK, but ChangeLog entry needs work.
> 
> > Source/WebCore/ChangeLog:6
> > +        [DOM] Core part patch for supporting battery status event
> > +        https://bugs.webkit.org/show_bug.cgi?id=63376
> 
> "Add JavaScript wrappers for battery status events." would read better IMO. Also, things in brackets are usually used for port names. Since this is isn't port-specific, you can remove it.
> 
> > Source/WebCore/ChangeLog:14
> > +        Add Core part modification for battery status event
> > +        - JSEventCustom.cpp, V8EventCustom.cpp
> > +          binding event between JavaSctipt and WebCore
> > +        - dom/Document.cpp Event.cpp Event.h Eventnames.h
> > +          Define and create event.
> > +        - Page.h Page.cpp, DOMWindow.h DOMWindow.cpp
> > +          Add/Remove event listener and init event handler to pageClient
> 
> These should go below, right after each file name.
> 
> > Source/WebCore/ChangeLog:18
> > +        no new tests.
> > +        This event implementation is not finished with this patch.
> > +        After finishing inplemetation, It will be added.
> 
> Begin sentences with a capital letter. Better yet, rephrase this to say something like "Test cases will be added as soon as the whole battery status feature is implemented.".
Comment 8 Leandro Pereira 2011-06-29 06:38:14 PDT
Comment on attachment 98840 [details]
Modify description in the ChangeLog

View in context: https://bugs.webkit.org/attachment.cgi?id=98840&action=review

> Source/WebCore/ChangeLog:44
> +        * bindings/js/JSEventCustom.cpp:
> +        (WebCore::toJS):
> +        * bindings/v8/custom/V8EventCustom.cpp:
> +        (WebCore::toV8):
> +        * dom/Document.cpp:
> +        (WebCore::Document::createEvent):
> +        * dom/Event.cpp:
> +        (WebCore::Event::isBatteryStatusEvent):
> +        * dom/Event.h:
> +        * dom/EventNames.h:
> +        * page/DOMWindow.cpp:
> +        (WebCore::DOMWindow::addEventListener):
> +        (WebCore::DOMWindow::removeEventListener):
> +        (WebCore::DOMWindow::removeAllEventListeners):
> +        * page/DOMWindow.h:
> +        * page/Page.cpp:
> +        (WebCore::Page::Page):
> +        (WebCore::Page::PageClients::PageClients):
> +        * page/Page.h:
> +        (WebCore::Page::batteryStatusController):

These are added so that you can fill them; no need for a separate section above, like so:

* page/Page.cpp: Add controller and client for...
...
* page/Page.cpp: Ditto.
Comment 9 Kihong Kwon 2011-07-01 02:15:13 PDT
Created attachment 99446 [details]
Modify ChangeLog's comments
Comment 10 Kihong Kwon 2011-07-01 03:49:22 PDT
Created attachment 99454 [details]
Modify comment of ChangLog
Comment 11 Kihong Kwon 2011-07-06 23:43:12 PDT
Created attachment 99944 [details]
Add events for the revised spec.

http://dev.w3.org/2009/dap/system-info/battery-status.html
Comment 12 Eric Seidel 2012-02-16 14:05:12 PST
It seems there isn't much interest in this feature.
Comment 13 Kihong Kwon 2012-02-17 00:04:57 PST
(In reply to comment #12)
> It seems there isn't much interest in this feature.

Battery Status API is almost close to CfC.
So. I will add a new patch for this soon.
Comment 14 Eric Seidel 2012-03-20 00:45:14 PDT
Comment on attachment 99944 [details]
Add events for the revised spec.

Cleared review? from attachment 99944 [details] so that this bug does not appear in http://webkit.org/pending-review.  If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again).
Comment 15 Kihong Kwon 2012-04-05 00:15:22 PDT
Battery status API has already landed by bug #62698.