467071 - new embedding API observer addons/extensions

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[Tobias Hunger]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111616 Ubuntu/9.04 (jaunty) Firefox/3.0.4
Build Identifier: incubator/embedding hg repository rev. dabab67820e6

The attached patch adds fixes for lots of build warnings in the new embedding API, adds copyright headers where not present already and extends the existing Listener with a couple of new callbacks:

 * Adds a bool to DocumentLoaded, indicating sucess or failure of the load operation
 * Adds FrameLoaded() callback.
 * Adds AttachAdditionalObservers() which will be used later to attach additional observers to DOM elements.
 * DocumentLoadStarted() to signal (guess what) start of a document load.
 * MoveTo() to notify about (window-) move events.

These extensions are required to build a full featured client based on the new embedding APIs. Please consider this patch for incubator.

Possible issues:

We only tested this patch with the Qt flavor of the embedding APIs.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Tobias Hunger]

Attachment: The actual patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Uh... What's the format of that patch, exactly?  It doesn't seem to be a diff to me.  Please attach a diff if at all possible, so it can just be read in a web browser.

Comment 3

[Tobias Hunger]

Sorry, it is a gzip'ed diff...

I checked that the mime type was set properly when uploading the patch, I'll fix this.

Comment 4

[Tobias Hunger]

Attachment: The actual patch (not gzip'ed)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Tobias, thanks.

The reason I wanted to look at the patch was because I wanted to see what definition of "document load succeeded" was being used here.  As far as I can tell, a 404 HTTP response sounds as a successful load here, right?  And a load that never manages to create a channel doesn't even hit this code?

None of that is clear from the API documentation.

In general, it's worth splitting out substantive changes like that from the whitespace cleanup, bracing changes (some a little suspect, but up to the module owner here), etc, etc...  At least if this code is expected to get code review (which I most certainly hope it is at some point!).

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Tobias, thanks for the patches. Some formatting and style feedback after a quick look:

* 4 spaces in c/c++ files for indentation, no tabs
* one space between if/for/switch and (
* no space between function name and (
* no else after return
* return early for failure 

I know that the code you patched against did not follow most of these guidelines, but we just landed a patch that started the fixup process. I figure you could adjust your code we possible. Whitespace is a lower priority (I can fix that up quickly), but more important is:

* no else after return
* return early for failure 

Your code adds some of these patterns:

 NS_IMETHODIMP WebBrowserChrome::SetStatus(PRUint32 statusType, const PRUnichar *status)
 {
-    MozViewListener* pListener = pMozView->GetListener();
-    if (!pListener)
-        return NS_ERROR_NOT_IMPLEMENTED;
-

Keep this (you change this pattern a lot)

+  MozViewListener* pListener = pMozView->GetListener();
+  if(pListener) {

space between "if" and "("


NS_IMETHODIMP WebBrowserChrome::OnProgressChange(nsIWebProgress *aWebProgress, nsIRequest *aRequest, PRInt32 aCurSelfProgress, PRInt32 aMaxSelfProgress, PRInt32 aCurTotalProgress, PRInt32 aMaxTotalProgress)
 {
+  MOZ_UNUSED(aWebProgress);
+  MOZ_UNUSED(aRequest);
+  MOZ_UNUSED(aCurSelfProgress);
+  MOZ_UNUSED(aMaxSelfProgress);
+
+  MozViewListener* pListener = pMozView->GetListener();
+  if(pListener)
+  {
+      PRUint32 percentage = ( 100 * aCurTotalProgress ) / aMaxTotalProgress;
+      if (percentage > 100) { percentage = 100; }
+      pListener->ProgressChanged(percentage);
+      return NS_OK;
+  }
+  else {
     return NS_ERROR_NOT_IMPLEMENTED;
+  }
 }

Don't add an "else" here. Just use the "return" without the "else"
==================================================================

You have a lot of good stuff in this patch (and the other ones you files too). We will be reviewing and landing what we can. Thanks again.

Comment 7

[Tobias Hunger]

Boris: Yes, I agree that it would have been nice to send in the patches in a more granular fashion. Unfortunately that proved harder to do than expected. In the end I went for this approach since otherwise I'd end up with patches that are even more dependent on the order in which they should be applied than what I send in. And I found it harder to review a series of interdependent patches than to just ignore the whitespace changes.

About the document loaded remarks: You are right, documentation should be improved here (I'll do that ASAP).

Mark: I'll clean up the patch and resubmit it.

Comment 8

[Tobias Hunger]

Attachment: observer extensions patch v1.1

This patch incorparates the suggestions made by reviewers.

Comment 9

[Boris Zbarsky [:bzbarsky]]

I'm not sure I follow the documentation for DocumentLoaded()...

Of course I'm also not sure what use cases aSuccess is expected to address.

Comment 10

[Anton Rogaynis (antonr)]

Following code should be changed 
+    if (!pListener) { return NS_ERROR_NOT_IMPLEMENTED; }

back to 

-    if (!pListener)
-        return NS_ERROR_NOT_IMPLEMENTED;

as Mark suggested, there are some places (~6) in the v1.1 patch.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #10)
> Following code should be changed 
> +    if (!pListener) { return NS_ERROR_NOT_IMPLEMENTED; }
> 
> back to 
> 
> -    if (!pListener)
> -        return NS_ERROR_NOT_IMPLEMENTED;

Yes. I fixed 3 of those in the patch for bug 467073 before checking in

Comment 12

[Tobias Hunger]

Is there anything I should do to make the patch more acceptable?

I do personaly consider if statements without brackets to be a oversight, so I corrected that wherever I ran into it:-)

Comment 13

[Tobias Hunger]

Attachment: Rework to avoid changing if (!pListener)

Rework the patch to avoid introducing { } around single statement if constructs.

Split out removal of unused Embedding.h header, too.

Comment 14

[Tobias Hunger]

Attachment: Update copyright year to 2007 instead of 20007.

Update copyright year to 2007 instead of 20007.

Otherwise the patch is identical with the previous one.

Comment 15

[Tobias Hunger]

Attachment: observer extensions patch v1.2

Shrink the patch by depending on cleanup patches...

Comment 16

[Tobias Hunger]

OK, the important changes should be more obvious now.

This patch further adds some Qt code to demonstrate the use (this code was part of #467076 before, mixed in with lots of other stuff:-).

Comment 17

[Tobias Hunger]

Attachment: observer extensions patch v1.3

This patch applies to current hg.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 353429 [details] [diff] [review]
observer extensions patch v1.3

Some changes I'd to see:

I want to change our event names to a more event-like name. Since you touch some in this patch let's start:
* ProgressChanged -> OnProgressChanged
* FrameLoaded -> OnFrameLoaded
* DocumentLoaded -> OnDocumentLoaded
* DocumentLoadStarted -> OnDocumentLoading

Why the need for AttachAdditionallisteners? I'd rather remove it for now.

Overall this looks good. Thanks

Comment 19

[Tobias Hunger]

Attachment: observer extensions patch v1.4

* Rename methods to start with "On" as suggested by Mark.
  Qt Signals were left alone as a signal implies that something
  has happened and it is not usually marked by prefixes as "On".
* Update to current mercurial version.
* Depend on #470201 to shrink the patch a bit and to not
  redo so many changes.

Comment 20

[Tobias Hunger]

Attachment: observer extensions patch v1.5

Update patch to new hg revision (plus the two fixes found on bug #470201).

Comment 21

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.