Bug 63376 – [DOM] Core part patch for supporting battery status event

• Component: DOM
• Status: RESOLVED INVALID

[Kihong Kwon]

This patch is a core part patch for battery status event(related bug 62698)
have to be patched after bug 62698

[Kihong Kwon]

Created attachment 98579 [details]
Add core patch files for the battery status event

[Gyuyoung Kim]

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

[Kihong Kwon]

Created attachment 98581 [details]
Add core patch files for the battery status event.

[Kihong Kwon]

Created attachment 98651 [details]
Add core patch files for the battery status event

[Leandro Pereira]

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.".

[Kihong Kwon]

Created attachment 98840 [details]
Modify description in the ChangeLog

[Kihong Kwon]

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.".

[Leandro Pereira]

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.

[Kihong Kwon]

Created attachment 99446 [details]
Modify ChangeLog's comments

[Kihong Kwon]

Created attachment 99454 [details]
Modify comment of ChangLog

[Kihong Kwon]

Created attachment 99944 [details]
Add events for the revised spec.

http://dev.w3.org/2009/dap/system-info/battery-status.html

[Eric Seidel (no email)]

It seems there isn't much interest in this feature.

[Kihong Kwon]

(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.

[Eric Seidel (no email)]

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).

[Kihong Kwon]

Battery status API has already landed by bug #62698.