662806 - nsINavHistoryObserver: pass GUID to onPageChanged, onTitleChanged

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Philipp von Weitershausen [:philikon]]

Left-over from bug 633266 that turned out to be pretty hairy and not needed for Sync.

Comment 1

[Marco Bonardo [:mak]]

Ideally, if we may break observers compatibility in just a single version would be better than spreading across 2 versions.

Comment 2

[Marco Bonardo [:mak]]

Attachment: onTitleChanged v1.0

first part of the bug

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 541922 [details] [diff] [review]
onTitleChanged v1.0

>+add_test(function test_onTitleChanged() {
>+  onNotify(function onTitleChanged(aURI, aTitle, aGUID) {
>+    do_check_true(aURI.equals(testuri));
>+    // Can't use do_check_guid_for_uri() here because the visit is already gone.

While this is true within the onDeleteVisit handler above, it shouldn't be an issue in onTitleChanged.

>+    do_check_eq(aTitle, title);
>+    do_check_eq(aGUID, testguid);
>+
>+    run_next_test();
>+  });

Comment 4

[Marco Bonardo [:mak]]

(In reply to comment #3)
> Comment on attachment 541922 [details] [diff] [review] [review]
> onTitleChanged v1.0
> 
> >+add_test(function test_onTitleChanged() {
> >+  onNotify(function onTitleChanged(aURI, aTitle, aGUID) {
> >+    do_check_true(aURI.equals(testuri));
> >+    // Can't use do_check_guid_for_uri() here because the visit is already gone.

ehr copy/paste madness. thanks.

Comment 5

[Marco Bonardo [:mak]]

Attachment: onTitleChanged v1.1

fixed philikon's comment, now working on onPageChanged.

Comment 6

[Marco Bonardo [:mak]]

Attachment: OnPageChanged v1.0

the remaining notification.
The only drawback of the change is that when async favicons create a new page I now have to do an additional query to get its guid, but this is not the common operation, should be a rare case.

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 542241 [details] [diff] [review]
onTitleChanged v1.1

Review of attachment 542241 [details] [diff] [review]:
-----------------------------------------------------------------

Comment 8

[Marco Bonardo [:mak]]

hm I may have to partially revert changes in the pageChanged patch, particularly the use of GetFaviconSpecForIconString in nsNavHistoryResult.
Ideally we should use moz-anno:favicon uris here, we never did and looks like for some reason it still doesn't work. Since it's not depply related to this bug I'll just file it apart and leave it as it was.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 542441 [details] [diff] [review]
OnPageChanged v1.0

temporarily clearing review per previous comment.

Comment 10

[Marco Bonardo [:mak]]

Attachment: onPageChanged v1.1

This reverts icon updates as they were, I'll now file the bug apart to figure out why using moz-anno:favicon doesn't work here, but it's not needed to fix this bug.

Comment 11

[Marco Bonardo [:mak]]

Filed Bug 667905, sorry for noise.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 542469 [details] [diff] [review]
onPageChanged v1.1

Philipp, would you mind reviewing this part since Dietrich is PTO and you touched this code recently enough?
I'd really like to finish these interface changes before next merge.

Comment 13

[Marco Bonardo [:mak]]

Comment on attachment 542469 [details] [diff] [review]
onPageChanged v1.1

Robert, this is the last part of the interface changes, as I anticipated you on irc.
They are adding GUID to the 2 last notifications that were not yet getting it.

Comment 14

[Robert Strong (they/them - no direct email)]

Comment on attachment 542241 [details] [diff] [review]
onTitleChanged v1.1

>diff --git a/toolkit/components/places/nsINavHistoryService.idl b/toolkit/components/places/nsINavHistoryService.idl
>--- a/toolkit/components/places/nsINavHistoryService.idl
>+++ b/toolkit/components/places/nsINavHistoryService.idl
>@@ -804,17 +804,17 @@ interface nsINavHistoryResult : nsISuppo
> 
> /**
>  * Similar to nsIRDFObserver for history. Note that we don't pass the data
>  * source since that is always the global history.
>  *
>  * DANGER! If you are in the middle of a batch transaction, there may be a
>  * database transaction active. You can still access the DB, but be careful.
>  */
>-[scriptable, uuid(67cb2f78-555e-48c2-8df5-9af99a9ab3a4)]
>+[scriptable, uuid(b02e6e42-9ef1-4b89-9ea3-aae29244fcef)]
> interface nsINavHistoryObserver : nsISupports
> {
>   /**
>    * Notifies you that a bunch of things are about to change, don't do any
>    * heavy-duty processing until onEndUpdateBatch is called.
>    */
>   void onBeginUpdateBatch();
> 
>@@ -867,17 +867,18 @@ interface nsINavHistoryObserver : nsISup
>    * empty string in either case).
>    *
>    * @param aURI
>    *        The URI of the page.
>    * @param aPageTitle
>    *        The new title of the page.
>    */
>   void onTitleChanged(in nsIURI aURI,
>-                      in AString aPageTitle);
>+                      in AString aPageTitle,
>+                      in ACString aGUID);
Please add the new aGUID param to the comment

Comment 15

[Marco Bonardo [:mak]]

Attachment: onTitleChanged v1.2

Bah, sorry, I should use my brain sometimes :(

Comment 16

[Marco Bonardo [:mak]]

Attachment: onPageChanged v1.2

this was missing the uuid change too. Not a big deal in this case since the other patch has it, but wrong still.

Comment 17

[Philipp von Weitershausen [:philikon]]

Comment on attachment 542964 [details] [diff] [review]
onPageChanged v1.2

>diff --git a/toolkit/components/places/AsyncFaviconHelpers.cpp b/toolkit/components/places/AsyncFaviconHelpers.cpp
>--- a/toolkit/components/places/AsyncFaviconHelpers.cpp
>+++ b/toolkit/components/places/AsyncFaviconHelpers.cpp
>@@ -144,22 +144,23 @@ FetchPageInfo(StatementCache<mozIStorage
>   PRBool isNull;
>   stmt->GetIsNull(1, &isNull);
>   NS_ENSURE_SUCCESS(rv, rv);

As discussed on IRC: need to actually assign return value to rv here (and in other similar places).

> // nsFaviconService::GetFaviconSpecForIconString
> //
> //    This computes a favicon spec for when you don't want a URI object (as in
> //    the tree view implementation), sparing all parsing and normalization.
> void
>-nsFaviconService::GetFaviconSpecForIconString(const nsCString& aSpec,
>+nsFaviconService::GetFaviconSpecForIconString(const nsACString& aSpec,
>                                               nsACString& aOutput)
...
> 
>   // addition to API for strings to prevent excessive parsing of URIs
>   nsresult GetFaviconLinkForIconString(const nsCString& aIcon, nsIURI** aOutput);
>-  void GetFaviconSpecForIconString(const nsCString& aIcon, nsACString& aOutput);
>+  void GetFaviconSpecForIconString(const nsACString& aIcon, nsACString& aOutput);

This seems like an unrelated change.

>   /**
>-   * A page has had some attribute on it changed. Note that for TYPED and
>-   * HIDDEN, the page may not necessarily have been added yet.
>+   * Favicon has been updated.  onPageChanged will get the icon spec.
>+   */

"onPageChanged will get the icon spec." could be a bit clearer, e.g. "onPageChanged will be notified with the new favicon URI string for the aNewValue parameter."


Looks good otherwise.

Comment 18

[Robert Strong (they/them - no direct email)]

Comment on attachment 542964 [details] [diff] [review]
onPageChanged v1.2

>diff --git a/toolkit/components/places/nsINavHistoryService.idl b/toolkit/components/places/nsINavHistoryService.idl
>--- a/toolkit/components/places/nsINavHistoryService.idl
>+++ b/toolkit/components/places/nsINavHistoryService.idl
>@@ -804,17 +804,17 @@ interface nsINavHistoryResult : nsISuppo
> 
> /**
>  * Similar to nsIRDFObserver for history. Note that we don't pass the data
>  * source since that is always the global history.
>  *
>  * DANGER! If you are in the middle of a batch transaction, there may be a
>  * database transaction active. You can still access the DB, but be careful.
>  */
>-[scriptable, uuid(b02e6e42-9ef1-4b89-9ea3-aae29244fcef)]
>+[scriptable, uuid(c837f6ba-6ad7-4810-a425-8ce29e05d17e)]
> interface nsINavHistoryObserver : nsISupports
> {
>   /**
>    * Notifies you that a bunch of things are about to change, don't do any
>    * heavy-duty processing until onEndUpdateBatch is called.
>    */
>   void onBeginUpdateBatch();
> 
>@@ -923,30 +923,36 @@ interface nsINavHistoryObserver : nsISup
>                    in unsigned short aReason);
> 
>   /**
>    * Notification that all of history is being deleted.
>    */
>   void onClearHistory();
> 
>   /**
>-   * A page has had some attribute on it changed. Note that for TYPED and
>-   * HIDDEN, the page may not necessarily have been added yet.
>+   * Favicon has been updated.  onPageChanged will get the icon spec.
>+   */
>+  const unsigned long ATTRIBUTE_FAVICON = 3;
>+
>+  /**
>+   * An attribute of this page changed.
>    *
>    * @param aURI
>-   *         The URI of the page on which an attribute changed.
>+   *        The URI of the page on which an attribute changed.
>    * @param aWhat
>-   *         The attribute whose value changed.
>-   * @param aValue
>-   *         The attribute's new value. 
>+   *        The attribute whose value changed.  See ATTRIBUTE_* constants.
>+   * @param aNewValue
>+   *        The attribute's new value.
>+   * @param aGUID
>+   *        The unique ID associated with the page.
>    */
>-  const unsigned long ATTRIBUTE_FAVICON = 3; // favicon updated, aString = favicon annotation URI
>   void onPageChanged(in nsIURI aURI,
>                      in unsigned long aWhat,
>-                     in AString aValue);
>+                     in AString aNewValue,
>+                     in ACString aGUID);
Not part of this review but I really think using aWhat as a param is lazy :(

Comment 19

[Marco Bonardo [:mak]]

you mean as a name for the variable, or for the interface?
the interface sucks a bit but I don't want to break it for js consumers for now.

Comment 20

[Marco Bonardo [:mak]]

clarified on IRC, I'll change aWhat to aChangedAttribute

Comment 21

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/aff52f424fa6
http://hg.mozilla.org/projects/places/rev/e4095d0a9ac4

Comment 22

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/e4095d0a9ac4
http://hg.mozilla.org/mozilla-central/rev/aff52f424fa6

Comment 23

[Hussam Al-Tayeb]

I am now getting a build error: 
/home/hussam/packages/firefox/toolkit/components/places/nsNavHistoryResult.cpp: In member function ‘virtual nsresult nsNavHistoryQueryResultNode::OnPageChanged(nsIURI*, PRUint32, const nsAString_internal&, const nsACString_internal&)’:
/home/hussam/packages/firefox/toolkit/components/places/nsNavHistoryResult.cpp:3163:55: error: taking address of temporary [-fpermissive]

I was told on irc that it is due to this bug.

Comment 24

[Marco Bonardo [:mak]]

(In reply to comment #23)
> I was told on irc that it is due to this bug.

Yeah, makes sense, the compiler didn't save me.

Comment 25

[Marco Bonardo [:mak]]

reverted the stupid change, sorry.
http://hg.mozilla.org/mozilla-central/rev/3cb00dc5d94b