Closed Bug 662806 Opened 10 years ago Closed 9 years ago

nsINavHistoryObserver: pass GUID to onPageChanged, onTitleChanged

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla7

People

(Reporter: philikon, Assigned: mak)

References

Details

(Whiteboard: [places-next-wanted][fixed-in-places])

Attachments

(2 files, 4 obsolete files)

Left-over from bug 633266 that turned out to be pretty hairy and not needed for Sync.
Ideally, if we may break observers compatibility in just a single version would be better than spreading across 2 versions.
Whiteboard: [places-next-wanted]
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Flags: in-testsuite?
Attached patch onTitleChanged v1.0 (obsolete) — Splinter Review
first part of the bug
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();
>+  });
(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.
Attached patch onTitleChanged v1.1 (obsolete) — Splinter Review
fixed philikon's comment, now working on onPageChanged.
Attachment #541922 - Attachment is obsolete: true
Attachment #542241 - Flags: review?(dietrich)
Attached patch OnPageChanged v1.0 (obsolete) — Splinter Review
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.
Attachment #542441 - Flags: review?(dietrich)
Whiteboard: [places-next-wanted] → [places-next-wanted][needs-sr]
Comment on attachment 542241 [details] [diff] [review]
onTitleChanged v1.1

Review of attachment 542241 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #542241 - Flags: review?(dietrich) → review+
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 on attachment 542441 [details] [diff] [review]
OnPageChanged v1.0

temporarily clearing review per previous comment.
Attachment #542441 - Flags: review?(dietrich)
Attached patch onPageChanged v1.1 (obsolete) — Splinter Review
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.
Attachment #542441 - Attachment is obsolete: true
Attachment #542469 - Flags: review?(dietrich)
Filed Bug 667905, sorry for noise.
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.
Attachment #542469 - Flags: review?(dietrich) → review?(philipp)
Attachment #542241 - Flags: superreview?(robert.bugzilla)
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.
Attachment #542469 - Flags: superreview?(robert.bugzilla)
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
Attachment #542241 - Flags: superreview?(robert.bugzilla) → superreview-
Bah, sorry, I should use my brain sometimes :(
Attachment #542241 - Attachment is obsolete: true
Attachment #542963 - Flags: superreview?(robert.bugzilla)
this was missing the uuid change too. Not a big deal in this case since the other patch has it, but wrong still.
Attachment #542469 - Attachment is obsolete: true
Attachment #542469 - Flags: superreview?(robert.bugzilla)
Attachment #542469 - Flags: review?(philipp)
Attachment #542964 - Flags: superreview?(robert.bugzilla)
Attachment #542964 - Flags: review?(philipp)
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.
Attachment #542964 - Flags: review?(philipp) → review+
Attachment #542963 - Flags: superreview?(robert.bugzilla) → superreview+
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 :(
Attachment #542964 - Flags: superreview?(robert.bugzilla) → superreview+
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.
clarified on IRC, I'll change aWhat to aChangedAttribute
http://hg.mozilla.org/projects/places/rev/aff52f424fa6
http://hg.mozilla.org/projects/places/rev/e4095d0a9ac4
Whiteboard: [places-next-wanted][needs-sr] → [places-next-wanted][fixed-in-places]
http://hg.mozilla.org/mozilla-central/rev/e4095d0a9ac4
http://hg.mozilla.org/mozilla-central/rev/aff52f424fa6
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Flags: in-testsuite? → in-testsuite+
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.
(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.
reverted the stupid change, sorry.
http://hg.mozilla.org/mozilla-central/rev/3cb00dc5d94b
You need to log in before you can comment on or make changes to this bug.