Bug 79052

Summary: NOTIFICATIONS should be implemented as PageSupplement
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: PlatformAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, aroben, hausmann, jonlee, ossy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 78440    
Attachments:
Description Flags
Patch none

Description Hajime Morrita 2012-02-20 16:24:16 PST
This is series of supplement-ification (Bug 78440).
NotificationController and its client should be pushed out from Page class using PageSupplement.
Comment 1 Hajime Morrita 2012-02-20 16:25:53 PST
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.
Comment 2 Hajime Morrita 2012-02-22 22:08:42 PST
Created attachment 128395 [details]
Patch
Comment 3 Hajime Morrita 2012-02-22 22:09:34 PST
Adam, Hakaken, could you give me a stamp? This will help future supplimentalization.
Comment 4 WebKit Review Bot 2012-02-22 23:29:15 PST
Comment on attachment 128395 [details]
Patch

Clearing flags on attachment: 128395

Committed r108615: <http://trac.webkit.org/changeset/108615>
Comment 5 WebKit Review Bot 2012-02-22 23:29:20 PST
All reviewed patches have been landed.  Closing bug.
Comment 6 Csaba Osztrogonác 2012-02-23 00:34:14 PST
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’
Comment 7 Adam Barth 2012-02-23 00:40:51 PST
Why was the qt EWS bot green?  :(

/me will see if morrita is around and if not I'll fix it.
Comment 8 Simon Hausmann 2012-02-23 00:43:54 PST
(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.
Comment 9 Simon Hausmann 2012-02-23 00:53:40 PST
Committed r108621: <http://trac.webkit.org/changeset/108621>
Comment 10 Csaba Osztrogonác 2012-02-23 06:34:19 PST
(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.
Comment 11 Simon Hausmann 2012-02-23 06:39:28 PST
(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? :)
Comment 12 Adam Roben (:aroben) 2012-02-23 08:04:38 PST
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.
Comment 13 Hajime Morrita 2012-02-23 16:27:17 PST
(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.