Bug 79052 – NOTIFICATIONS should be implemented as PageSupplement

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Hajime Morrita]

This is series of supplement-ification (Bug 78440).
NotificationController and its client should be pushed out from Page class using PageSupplement.

[Hajime Morrita]

CC-ed some folks just for FYI because it looks notification is under hot discussion.
Note that this change never intents any functional change.
This is purely code-level factoring.

[Hajime Morrita]

Created attachment 128395 [details]
Patch

[Hajime Morrita]

Adam, Hakaken, could you give me a stamp? This will help future supplimentalization.

[WebKit Review Bot]

Comment on attachment 128395 [details]
Patch

Clearing flags on attachment: 128395

Committed r108615: <http://trac.webkit.org/changeset/108615>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

Reopen, because it broke Qt WK2 build:
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/WebPage/WebPage.cpp: In constructor ‘WebKit::WebPage::WebPage(uint64_t, const WebKit::WebPageCreationParameters&)’:
/home/webkitbuildbot/slaves/release32bit-qt5/buildslave/qt-linux-32-release-qt5/build/Source/WebKit2/WebProcess/WebPage/WebPage.cpp:236: error: ‘struct WebCore::Page::PageClients’ has no member named ‘notificationClient’

[Adam Barth]

Why was the qt EWS bot green?  :(

/me will see if morrita is around and if not I'll fix it.

[Simon Hausmann]

(In reply to comment #7)
> Why was the qt EWS bot green?  :(

Because the Qt EWS doesn't build WebKit2 :(

I hope that we can change that in the future.

[Simon Hausmann]

Committed r108621: <http://trac.webkit.org/changeset/108621>

[Csaba Osztrogonác]

(In reply to comment #8)
> (In reply to comment #7)
> > Why was the qt EWS bot green?  :(
> 
> Because the Qt EWS doesn't build WebKit2 :(
> 
> I hope that we can change that in the future.

The Qt EWS is same as "Qt Linux Release" bot on build.webkit.org.
(Qt 4.8.0, x86 architecture)

It is only a decision, we can do everything. :) Now all of our bots (except Qt Linux 64-bit Release (Perf)) on build.webkit.org uses the released Qt 4.8.0 because of historically reason. When we drop WebKit2 for Qt 4.8, Qt 5 wasn't so stable, we have build problems every day, etc.

If we would like to switch the EWS to Qt 5, we should guarantee that QtWebKit with Qt5 with WebKit2 almost always builds. If the EWS doesn't work for a night, because nobody fix the build, it can cause much more trouble than we don't have WebKit2 EWS and somebody break the Qt-WK2 build once in a month.

The best solution would be if we setup a Qt-WK2 EWS too and leave the Qt EWS build WebKit1 with Qt 4.8.

[Simon Hausmann]

(In reply to comment #10)
> (In reply to comment #8)
> > (In reply to comment #7)
> > > Why was the qt EWS bot green?  :(
> > 
> > Because the Qt EWS doesn't build WebKit2 :(
> > 
> > I hope that we can change that in the future.
> 
> The Qt EWS is same as "Qt Linux Release" bot on build.webkit.org.
> (Qt 4.8.0, x86 architecture)
> 
> It is only a decision, we can do everything. :) Now all of our bots (except Qt Linux 64-bit Release (Perf)) on build.webkit.org uses the released Qt 4.8.0 because of historically reason. When we drop WebKit2 for Qt 4.8, Qt 5 wasn't so stable, we have build problems every day, etc.
> 
> If we would like to switch the EWS to Qt 5, we should guarantee that QtWebKit with Qt5 with WebKit2 almost always builds. If the EWS doesn't work for a night, because nobody fix the build, it can cause much more trouble than we don't have WebKit2 EWS and somebody break the Qt-WK2 build once in a month.
> 
> The best solution would be if we setup a Qt-WK2 EWS too and leave the Qt EWS build WebKit1 with Qt 4.8.

Right, either two EWSes or one Qt-EWS that does a build against Qt 4 and Qt 5 in one shot. I suppose the former is easier? :)

[Adam Roben (:aroben)]

This broke the Mac build:

Undefined symbols for architecture x86_64:
  "__ZN7WebCore19provideNotificationEPNS_4PageEPNS_21NotificationPresenterE", referenced from:
ld: symbol(s) not found for architecture x86_64

I'm fixing it now.

[Hajime Morrita]

(In reply to comment #12)
> This broke the Mac build:
> Undefined symbols for architecture x86_64:
>   "__ZN7WebCore19provideNotificationEPNS_4PageEPNS_21NotificationPresenterE", referenced from:
> ld: symbol(s) not found for architecture x86_64
> 
> I'm fixing it now.
Oops. Thank you for taking care of this.