Closed Bug 566738 Opened 14 years ago Closed 14 years ago

Add SetURITitle to IHistory

Categories

(Toolkit :: Places, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla2.0b2
Tracking Status
blocking2.0 --- beta2+
fennec 2.0+ ---

People

(Reporter: stechz, Assigned: stechz)

References

Details

Attachments

(1 file, 6 obsolete files)

Docshell will now call IHistory::SetTitle, which will asynchronously change the page title associated with a place.
Changed name to SetURITitle.
Summary: Add SetTitle to IHistory → Add SetURITitle to IHistory
Component: General → Places
Product: Fennec → Toolkit
QA Contact: general → places
We going for this:
class IHistory {

...

NS_IMETHOD SetURITitle(nsIURI *aTitle,
                       nsString aTitle);
}

?
|const nsString& aTitle|?

Also, do you expect anyone outside Gecko propert to call it?  I so, don't you need nsAString?
(In reply to comment #3)
> |const nsString& aTitle|?
Errr, yes.

> Also, do you expect anyone outside Gecko propert to call it?  I so, don't you
> need nsAString?
I don't expect anyone outside of gecko to call anything on IHistory, so we can get away with the non-abstract string classes, yeah?
Yes, if only internal-api code will be calling it.
This should block the beta for 1.9.3 since fennec needs this for e10s.
blocking2.0: --- → ?
tracking-fennec: --- → ?
tracking-fennec: ? → 1.1+
just a note, this actually blocks fennec 2.0, but that's not an option in our multi-flag right now. I've filed bug 568391 to get it added and marked this as blocking 1.1 for the time being so we don't loose track of it.
tracking-fennec: 1.1+ → 2.0+
Attached patch Patch (obsolete) — Splinter Review
Based on work from 556400.
Assignee: nobody → webapps
Attachment #447820 - Flags: review?
Attachment #447820 - Flags: review? → review?(mak77)
Depends on: 556400
Comment on attachment 447820 [details] [diff] [review]
Patch

>diff --git a/docshell/base/IHistory.h b/docshell/base/IHistory.h

>+
>+    /**
>+     * Set the title of the URI.
>+     *
>+     * @pre aURI must be null.

I hope the opposite?


>+     * @param aURI
>+     *        Set the title for this URI.

The URI we will set the title for.


>diff --git a/toolkit/components/places/src/History.cpp b/toolkit/components/places/src/History.cpp

>+/**
>+ * Step 3: Notify that title has been updated.
>+ */

I have not checked the last patch, but iirc Shawn asked you to reverse order of the steps in the visitURI patch so they were in order?
If so should be done here too, otherwise nvm. but this order makes reviews a bit harder.


>+  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)
>+  {
>+    nsNavHistory* history = nsNavHistory::GetHistoryService();
>+    history->FireTitleChange(mData->uri, mData->title);

hu, can we s/Fire/Notify/? peace and love :)


>+  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)
>+  {
>+    NS_ENSURE_STATE(aResultSet);

when could this happen? If it's just a check for our internal use should be a NS_PRECONDITION (debug only)


>+NS_IMETHODIMP
>+History::SetURITitle(nsIURI* aURI, const nsString& aTitle)
>+{

>+
>+  if (aTitle.IsEmpty()) {
>+    data->title.SetIsVoid(PR_TRUE);

hm, actually a title like "" is different from a NULL title for us, but I'd guess we don't care in this case, null should be more efficient, so your choice seems fine.


>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>+void
>+nsNavHistory::FireTitleChange(nsIURI* aURI, const nsString& aTitle)
>+{
>+  // observers (have to check first if it's bookmarked)

This comment makes no sense, the interesting thing is that old code has it as well... can you nuke both please?

>diff --git a/toolkit/components/places/src/nsNavHistory.h b/toolkit/components/places/src/nsNavHistory.h

>+  /**
>+   * Fires onTitleChanged event to nsINavHistoryService observers
>+   */
>+  void FireTitleChange(nsIURI* aURI, const nsString& title);

ditto, I'm all for NotifyTitleChange


>diff --git a/toolkit/components/places/tests/browser/browser_settitle.js b/toolkit/components/places/tests/browser/browser_settitle.js

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
>+
>+/**
>+ * One-time observer callback.
>+ */
>+function waitForObserve(name, callback) {

better waitForTopic and first param should be aTopic


>+  var observerService = Cc["@mozilla.org/observer-service;1"]
>+                        .getService(Ci.nsIObserverService);

Services.obs is your friend


>+  var observer = {
>+    QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver]),
>+    observe: function(subject, topic, data) {
>+      observerService.removeObserver(observer, name);
>+      observer = null;

why is this useful?


>+/**
>+ * One-time DOMContentLoaded callback.
>+ */
>+function waitForLoad(callback) {
>+  gBrowser.addEventListener("DOMContentLoaded", function() {
>+    gBrowser.removeEventListener("DOMContentLoaded", arguments.callee, true);
>+    callback();
>+  }, true);
>+}

this method name is lying, it is not waiting "load" event!
it should probably also check that the load even comes for one of the pages we are loading... you know, running a bunch of tests in the same window can sometimes hurt (oranges are behind corner).


>+/**
>+ * Gets a single column value from either the places or historyvisits table.
>+ */
>+function getColumn(table, column, fromColumnName, fromColumnValue) {
>+  var stmt = conn.createStatement(
>+    "SELECT " + column + " FROM " + table + "_temp WHERE " + fromColumnName + "=:val " +
>+    "UNION ALL " +
>+    "SELECT " + column + " FROM " + table + " WHERE " + fromColumnName + "=:val " +
>+    "LIMIT 1");
>+  try {
>+    stmt.params.val = fromColumnValue;
>+    stmt.executeStep();
>+    return stmt.row[column];

what happens if it does not find any row?


>+  }
>+  finally {
>+    stmt.reset();

finalize() could be better


>+function test() {
>+  // Make sure places visit chains are saved correctly with a redirect
>+  // transitions.

This is another test, right?


>+  waitForExplicitFinish();
>+
>+  var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsINavHistoryService);

PlacesUtils.history


>+  // Create and add history observer.
>+  var historyObserver = {
>+    data: [],
>+    onBeginUpdateBatch: function() {},
>+    onEndUpdateBatch: function() {},
>+    onVisit: function(aURI, aVisitID, aTime, aSessionID, aReferringID,
>+                      aTransitionType) {
>+    },
>+    onTitleChanged: function(aURI, aPageTitle) {
>+      this.data.push({ uri: aURI, title: aPageTitle });
>+      ok(this.data.length <= 1, "Too many title changes!");
>+      if (this.data.length == 1)
>+        confirmResults(this.data);

you are loading 2 pages, should not we receive 2 notifications?


>+    },
>+    onBeforeDeleteURI: function(aURI) {},
>+    onDeleteURI: function(aURI) {},
>+    onClearHistory: function() {},
>+    onPageChanged: function(aURI, aWhat, aValue) {},
>+    onDeleteVisits: function() {},
>+    QueryInterface: XPCOMUtils.generateQI([Ci.nsINavHistoryObserver])

you can remove params for all unused methods


>+  hs.addObserver(historyObserver, false);

where are you removing this observer? recall that b-c tests don't run in sandboxes...
you should remove it before finishing, and also waitForClearHistory(finish).


>+  // Load begin page, click link on page to record visits.

where is the click part?


>+  content.location.href = "http://example.com/tests/toolkit/components/places/tests/browser/title1.html";
>+  waitForLoad(function() {
>+    content.location.href = "http://example.com/tests/toolkit/components/places/tests/browser/title2.html";
>+    waitForLoad(function() {
>+      content.location.href = "about:blank";

hm, I think a saner way to handle this is to open a new tab, run test in the new tab, and finally close it.
loading about:blank does not bring anything sane, it is specially managed in a bunch of code (See bug 554873 for example)


>+  function confirmResults(data) {
>+    is(data[0].uri.spec, "http://example.com/tests/toolkit/components/places/tests/browser/title2.html");
>+    is(data[0].title, "Some title");
>+
>+    data.forEach(function(item) {
>+      var title = getColumn("moz_places", "title", "url", data[0].uri.spec);
>+      is(title, item.title);
>+    });

should you check data length == 2 (or fix the above code)


The overall approach is fine, and I like how it has been implemented. just some glitches and the test to fix.
Attachment #447820 - Flags: review?(mak77) → review-
> I have not checked the last patch, but iirc Shawn asked you to reverse order of
> the steps in the visitURI patch so they were in order?

He sure did, but like I told you both before, that just doesn't work for inlined classes.  You both opted to stick with the current style instead of forward declaring the classes.

> If so should be done here too, otherwise nvm. but this order makes reviews a
> bit harder.

Yeah, sorry about that :(


> hm, actually a title like "" is different from a NULL title for us, but I'd
> guess we don't care in this case, null should be more efficient, so your choice
> seems fine.

Just following the old code.

> This comment makes no sense, the interesting thing is that old code has it as
> well... can you nuke both please?

Oops, how'd that get there?

> why is this useful?

I don't think that's used in the test, it can be removed.

> what happens if it does not find any row?

The test fails :)

> you are loading 2 pages, should not we receive 2 notifications?

Nope!  If the title stays the same (from blank to blank), then there is no title notification (just like before, except now we have a test for this behavior).  I'll add a comment to say as much.

> you can remove params for all unused methods

I think this causes errors, like webprogresslistener, but I could try.

> should you check data length == 2 (or fix the above code)

1 is the correct length.  It is effectively checked for above (if there are more or less results than the expected, there will be a test failure).

Will get a new patch up soon.
> The URI we will set the title for.

I want to reiterate that this this policy of completeness for function documentation is silly.  Documenting these parameters adds absolutely nothing of value.  You are forcing people to add useless comments.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #447820 - Attachment is obsolete: true
Attachment #448112 - Flags: review?(sdwilsh)
On vacation for the next week!
Assignee: webapps → dougt
(In reply to comment #11)
> > The URI we will set the title for.
> 
> I want to reiterate that this this policy of completeness for function
> documentation is silly.  Documenting these parameters adds absolutely nothing
> of value.  You are forcing people to add useless comments.

this is for automatic documentation generation.
Comment on attachment 448112 [details] [diff] [review]
Patch v2

could you take a first review pass at this patch?
Attachment #448112 - Flags: review?(sdwilsh) → review?(mak77)
(umm... maybe that is what you already did!.  Is this r+'ed, and needs a sr?, or should sdwilsh give the final r+?)
(In reply to comment #16)
> (umm... maybe that is what you already did!.  Is this r+'ed, and needs a sr?,
> or should sdwilsh give the final r+?)
I should probably look at it still, but it likely won't take long if mak has already spotted any major issues.
blocking2.0: ? → beta1+
Comment on attachment 448112 [details] [diff] [review]
Patch v2

>diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp


>+  TitleNotifyStep(nsAutoPtr<SetTitleData> aData) : mData(aData)
>+  {
>+  }

fix code style as step 3:

XXXStep(nsAutoPtr<SetTitleData> aData)
: mData(aData)
{
}


>+
>+  SetTitleStep(nsAutoPtr<SetTitleData> aData) : mData(aData)
>+  {
>+  }

ditto


>+  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)

>+    nsCOMPtr<mozIStorageRow> row;
>+    nsresult rv = aResultSet->GetNextRow(getter_AddRefs(row));
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    nsString title;

should be a nsAutoString


>diff --git a/toolkit/components/places/src/nsNavHistory.cpp b/toolkit/components/places/src/nsNavHistory.cpp

>-nsNavHistory::FireOnVisit(nsIURI* aURI,
>+nsNavHistory::NotifyOnVisit(nsIURI* aURI,
>                           PRInt64 aVisitID,
>                           PRTime aTime,
>                           PRInt64 aSessionID,
>                           PRInt64 referringVisitID,
>                           PRInt32 aTransitionType)

indentation is now wrong


>-    FireOnVisit(aURI, *aVisitID, aTime, aSessionID, referringVisitID,
>+    NotifyOnVisit(aURI, *aVisitID, aTime, aSessionID, referringVisitID,
>                 aTransitionType);

ditto


>-  void FireOnVisit(nsIURI* aURI,
>+  void NotifyOnVisit(nsIURI* aURI,
>                    PRInt64 aVisitID,
>                    PRTime aTime,
>                    PRInt64 aSessionID,
>                    PRInt64 referringVisitID,
>                    PRInt32 aTransitionType);
> 

ditto


>diff --git a/toolkit/components/places/tests/browser/browser_settitle.js b/toolkit/components/places/tests/browser/browser_settitle.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/browser/browser_settitle.js
>@@ -0,0 +1,107 @@
>+/**
>+ * Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/
>+ */

Even if this is pretty fine, can we please add

Contributors:
  XYZ <xyz#zyx.com> (original author)

like in the common tri-license?


>+function finishAndCleanUp()
>+{
>+  gBrowser.removeCurrentTab();
>+  finish();
>+}

You should still add the waitForClearHistory function (search mxr) and do waitForClearHistory(finish); instead of a plain finish();


>+    onTitleChanged: function(aURI, aPageTitle) {
>+      this.data.push({ uri: aURI, title: aPageTitle });
>+
>+      // We only expect one title change.
>+      //
>+      // Although we are loading two different pages, the first page does not
>+      // have a title.  Since the title starts out as empty and then is set
>+      // to empty, there is no title change notification.
>+      //
>+      ok(this.data.length <= 1, "Not too many title changes");
>+      if (this.data.length == 1) {
>+        PlacesUtils.history.removeObserver(this);

this presumes we won't ever receive more than 1 event since we immediately remove the observer when length == 1, the previous check looks like useless then. We should check that the first notification we get is for title of the second page, but that is already done later in check results.
This pretty much fails to test if we get multiple notifications for the same page, but that could be slow and tricky (would assume to check after a timeout, that is fragile).
I think we can retain the test as it is, just remove that useless length <= 1 check.


It looks fine globally, Shawn could still do a pass on it, i have finished here.
Attachment #448112 - Flags: review?(mak77) → review+
(In reply to comment #18)
> Even if this is pretty fine, can we please add
> 
> Contributors:
>   XYZ <xyz#zyx.com> (original author)
> 
> like in the common tri-license?
Why?  That's only useful if we ever want to re-license, but I don't think we can do that with the public domain (and the license policy page doesn't say we can or should do that).  Unless we hear from gerv that it's OK, I don't think we should be inventing our own license block header.
Was pretty much a shortcut to avoid having to check blame to know who should I ask to about a test... Doubt adding contributors would create issues to the license.
PS: I've seen other PD license headers in mxr that report contributors list.
Attachment #448112 - Flags: review?(sdwilsh)
(In reply to comment #20)
> Was pretty much a shortcut to avoid having to check blame to know who should I
> ask to about a test... Doubt adding contributors would create issues to the
> license.
We should still check on this.
Assignee: doug.turner → webapps
Comment on attachment 448112 [details] [diff] [review]
Patch v2

>+    /**
>+     * Set the title of the URI.
>+     *
>+     * @pre aURI must not be null.
>+     *
>+     * @param aURI
>+     *        The URI to set the title for.
>+     * @param aTitle
>+     *        The title string.
>+     */
>+    NS_IMETHOD SetURITitle(nsIURI* aURI, const nsString& aTitle) = 0;
Need to rev cid on this interface.

>+    if (mCurrentURI && mLoadType != LOAD_ERROR_PAGE) {
>+        IHistory* history = nsContentUtils::GetHistory();
We can't use nsContentUtils here, but I think you made a getter for this elsewhere.
>+        if (history) {
>+            history->SetURITitle(mCurrentURI, nsString(mTitle));
>+        }
>+        else if (mGlobalHistory) {
>+            mGlobalHistory->SetPageTitle(mCurrentURI, nsString(mTitle));
>+        }
No reason to do nsString(mTitle) for either case.  No clue why the old code did that.

>+  TitleNotifyStep(nsAutoPtr<SetTitleData> aData) : mData(aData)
>+  {
>+  }
nit: this format is not like the rest of the file (specifically, construct initialization of variables).  Please make sure all of your new classes follow the format of the file (see VisitedQuery).

>+/**
>+ * Step 2: Set title.
>+ */
>+class SetTitleStep : public Step
>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+
>+  SetTitleStep(nsAutoPtr<SetTitleData> aData) : mData(aData)
>+  {
>+  }
>+
>+  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)
>+  {
>+    NS_PRECONDITION(aResultSet, "No result set for getting the title");
>+
>+    nsCOMPtr<mozIStorageRow> row;
>+    nsresult rv = aResultSet->GetNextRow(getter_AddRefs(row));
>+    NS_ENSURE_SUCCESS(rv, rv);
We should assert the length of the results is only one here (GetNumEntries(PRUint32*))

>+    // It is actually common to set the title to be the same thing it used to
>+    // be. For example, going to any web page will always cause a title to be set,
>+    // even though it will often be unchanged since the last visit. In these
>+    // cases, we can avoid DB writing and observer overhead.
Really really sucks that we have to do this :(

>+#include "nsString.h"
> #include "mozilla/IHistory.h"
> #include "mozilla/dom/Link.h"
> #include "nsTHashtable.h"
>-#include "nsString.h"
We should just do a |class nsString;| in IHistory.h so other consumers don't also have to worry about this.  Having to have the proper order of includes sucks.

trusting mak's review on the tests.  r=sdwilsh with comments addressed.
Attachment #448112 - Flags: review?(sdwilsh) → review+
> >+  NS_IMETHOD Callback(mozIStorageResultSet* aResultSet)
> >+  {
> >+    NS_ENSURE_STATE(aResultSet);
> 
> when could this happen? If it's just a check for our internal use should be a
> NS_PRECONDITION (debug only)

Actually, if there are no results for getting the URI record aResultSet is null.  This is totally possible, so I'm changing this to return NS_OK if it is null.

> We should assert the length of the results is only one here
> (GetNumEntries(PRUint32*))

Why?  It's guaranteed by a constraint that no two records have the same URI.  We don't assert for this anywhere else either.
Attached patch Patch v3 (obsolete) — Splinter Review
Addresses comments except for what I mentioned above.
Attachment #448112 - Attachment is obsolete: true
Attachment #449730 - Flags: review+
(In reply to comment #24)
> Actually, if there are no results for getting the URI record aResultSet is
> null.  This is totally possible, so I'm changing this to return NS_OK if it is
> null.
Uh, yeah.  I think I missed that this was not a HandleResult method.

> Why?  It's guaranteed by a constraint that no two records have the same URI. 
> We don't assert for this anywhere else either.
Mostly asking for it to make sure we don't forget to filter out temp table stuff really.  We've had a bunch of subtle bugs with this stuff in the past, so it's just being extra careful in debug builds.
> Mostly asking for it to make sure we don't forget to filter out temp table
> stuff really.  We've had a bunch of subtle bugs with this stuff in the past, so
> it's just being extra careful in debug builds.

Actually, the query has LIMIT 1 in it like all the other ones, so I don't understand why this one is any different than the others.
(In reply to comment #27)
> Actually, the query has LIMIT 1 in it like all the other ones, so I don't
> understand why this one is any different than the others.
Likely because I missed it on the others too (big patches tend to have stuff get missed by reviewers, sadly).  You can make a debug-only macro for this if you want and sprinkle it in the right places.  Something like:
CHECK_RESULT_LENGTH(aResultSet, 1);
GetNumEntries is in statement, not results.  Not sure how you check number of entries with async statements?
(In reply to comment #29)
> GetNumEntries is in statement, not results.  Not sure how you check number of
> entries with async statements?
Errr, disregard all this.  I was looking at mozIStorageRow, which inherits from mozIStorageValueArray, but it doesn't make sense to check that.  My comments don't actually make sense...  Sorry about that.  I think I'm trying to do too much at once :/
Attachment #449730 - Flags: superreview?(bzbarsky)
Blocks: 499828
Mossop: do you think we need this for beta1 or can it wait until after?
if bug 556400 won't make b1 (ti is blocking b2), this can't make it as well.
Can't see why this needs to block b1, moving to b2 for now, but would like to understand what the blocking rationale is here in terms of products.
blocking2.0: beta1+ → beta2+
It's required for Fennec alpha 2.0.  An e10s patch depends on this bug landing.
Sorry for spam.  To be clear, Fennec 2.0 alpha 1.
Comment on attachment 449730 [details] [diff] [review]
Patch v3

>+++ b/docshell/base/IHistory.h
>+    NS_IMETHOD SetURITitle(nsIURI* aURI, const nsString& aTitle) = 0;

So I just realized... this makes it impossible to _implement_ IHistory outside our tree too, no?  I think we really need nsAString here.  :(

>+++ b/docshell/base/nsDocShell.cpp
>+            history->SetURITitle(mCurrentURI, nsString(mTitle));

mTitle is already an nsString.

>+            mGlobalHistory->SetPageTitle(mCurrentURI, nsString(mTitle));

And the cruft can go away here too.

>+++ b/toolkit/components/places/src/History.cpp
>@@ -300,17 +300,17 @@ public:
>+        history->NotifyOnVisit(mData->uri, visitId, mData->dateTime,
>                              mData->sessionId, mData->lastVisitId,
>                              mData->transitionType);

Fix the indent.

sr=bzbarsky with those changes.
Attachment #449730 - Flags: superreview?(bzbarsky) → superreview+
Depends on: 575886
Attached patch Check for shutting down (obsolete) — Splinter Review
Attachment #449730 - Attachment is obsolete: true
Attachment #455255 - Flags: review+
Comment on attachment 455255 [details] [diff] [review]
Check for shutting down


>+NS_IMETHODIMP
>+History::SetURITitle(nsIURI* aURI, const nsAString& aTitle)
>+{
>+  NS_PRECONDITION(aURI, "Must pass a non-null URI!");
>+  if (mShuttingDown) {
>+    return;
>+  }

this can't return void
Attached patch Fixes (obsolete) — Splinter Review
Attachment #455255 - Attachment is obsolete: true
Attachment #455306 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/07a9dcc28c5c
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3b2
Flags: in-testsuite+
Is this patch responsible for the huge (800%, 900%, etc) Ts Shutdown performance regressions?
I've backed this out along with bug 556400 due to one of them likely
being the cause of bug 576684.

http://hg.mozilla.org/mozilla-central/rev/c066080db735
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to comment #42)
> Is this patch responsible for the huge (800%, 900%, etc) Ts Shutdown
> performance regressions?
Possible, but it's been backed out.
Attached patch Green on try server (obsolete) — Splinter Review
Attachment #455306 - Attachment is obsolete: true
Attachment #457187 - Flags: review?(mak77)
Attachment #457187 - Attachment is obsolete: true
Attachment #457201 - Flags: review+
Attachment #457187 - Flags: review?(mak77)
ops, I messed up bug # in the push comment.
http://hg.mozilla.org/mozilla-central/rev/b2b4ec542eb3
Status: REOPENED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.