[Webkit-unassigned] [Bug 61140] [GTK] Add HTML5 Notifications support

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=61140





--- Comment #6 from Martin Robinson <mrobinson at webkit.org>  2011-05-31 08:36:52 PST ---
(In reply to comment #5)

> I was wondering if your share this point of view and if in this case a more traditional delegation architecture, using an interface to be implemented by client and a setter in WebView would be fine to push at least for this feature.

While that is the optimal approach in some ways, and we are using it for certain features soon, it pushes some burden onto the embedder. Implementing an interface typically involves writing lots of GObject boilerplate, which is a significantly  higher barrier than just adding a signal handler. 


> Ok. How do I define the comparator func so to compare on string content instead of address ?

If you Make a Vector<String> comparisons will automatically work. You should just be able to use .find(...) or whatever method exists on Vector. I believe it should just use the contained classes operator== overload.

> Gotta check and ping you to explain me all the subtlety behind that.

Sure, feel free.

> > > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:94
> > > +    gchar* origin = g_strdup(context->securityOrigin()->toString().utf8().data());
> > > +    g_signal_emit_by_name(m_webView, "notification-check-permission",
> > > +                          origin, &permission);
> > > +    g_free(origin);
> > 
> > Here you can just use context->securityOrigin()->toString().utf8().data() in the function call and avoid an entire string copy

> Tried that initially but somehow the address was rewritten sometime; thus the copy. Maybe the JS/GC thread do its magic backstage.

Hrm. As long as the signal is actually emitted during the function call g_signal_emit_by_name it should be fine. If you want to save a copy in the handler though, you'll need to do an actual memory copy.

> > > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:44
> > > +    char* replaceid;
> > replaceId should be replaceId, though perhaps just id would make sense?
> Well, the spec specifies the name replace id and this field is optional so 'id' might not convey the full intent.

Okay. replaceId is probably better then.

> > >> Source/WebKit/gtk/webkit/webkitwebview.h:85
> > >> +{
> > > 
> > > This { should be at the end of the previous line  [whitespace/braces] [4]
> > 
> > Please fix this.
> The enums around do not put opening brace on the same line, so should I ?
> BTW, do you think this enum should be located here or in webkiwebnotification.h ? I'd favor the 2nd solution.

Sorry. This is a public header so you should ignore this style error. We should fix the style bot to ignore these headers.

> See above. this code was not used by any port AFAIK. In later patch, I moved the implementation within the *Gtk and added empty methods in other ports (wx, windows, mac).
> Still we could reuse those impl, but thqt would imply a gchar* -> JSStringRef conversion each time we check for a permission.

For these sorts of changes, it'd be good if you could ping the original author to have a look as well.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.