885241 - Avoid GetPageTitle in bookmarkPage

Status: RESOLVED WORKSFORME

(Firefox :: Bookmarks & History, defect, P2)

Description

[Asaf Romano (gone)]

Attachment: patch

So here's the first sync-getPageTile->async-getPlaceInfo change we're going to do (See bug 834539 for the new API).

bookmarkPage uses getPageTitle as a fallback for error pages (that is, it tries to find a title from a past successful cases). So we need to use the new getPlaceInfo API, meaning we've to async-ify bookmarkPage as well. Even though it may seem an overkill for a fall-back case, I think it's a step in the right direction considering that all things done by this method should be async at some point (for example, the call to getMostRecentBookmarkForURI). Even more so, bookmarkPage already calls setCharsetForURI, which is async.

While reviewing the code I found some other issues, so I refactored the function a bit.

1. The comment about "full plugin pages" isn't true anymore (for a very long time, I think). You can try it on http://www.adobe.com/jp/events/cs3_web_edition_tour/swfs/perform.swf
1.1. Even if it was, the code would fail to handle it because the code attempts to use the content document anyway.
1.2. We got the "entered" URL from twice... once from the and once from browser.webNav.currentURI, and once from browser.currentURI, which is just a shortcut for browser.webNav.currentURI. Along with the browser.contentDocument.documentURL case for error pages, this rendered the code somewhat unreadable.

2. Error pages case:
2.1. The error-page charset was saved as the bookmarked page's charset
2.2. Same goes for description.

3. Removed some function names (I'm told they aren't necessary anymore).

4. It was possible for bookmarkPage to start a TM batch but never end it.

5. The description anno was saved even if there was no description available. This is bad given that it's going to be removed at some point.

6. NOT FIXED IN THIS PATCH: some things done by bookmarkPage don't seem right for the sidebar case. I think it's better to fix it by not using the panel in that case.

I'm pretty sure some browser tests needs to be updated to support the new async-nature of this method, so for now I'm only asking for feedback.

Comment 1

[Asaf Romano (gone)]

The final patch will also remove the getPageTitle usage from browser_privatebrowsing_placesTitleNoUpdate.js.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 765241 [details] [diff] [review]
patch

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

I think the approach may be fine

::: browser/base/content/browser-places.js
@@ +230,4 @@
>      this.panel.hidePopup();
>    },
>  
> +  beginBatch: function() {

please whitespace anonymous functions as function () instead of function()

@@ +253,5 @@
> +    return Task.spawn(function() {
> +      let uri = aBrowser.currentURI;
> +      let description = "", title = "", charset = "";
> +
> +      let doc = aBrowser.contentDocument;

if this should be executed asynchronously, may contentDocument go away before we can use it?

@@ +267,5 @@
> +      }
> +      else {
> +        title = doc.title;
> +        charset = doc.characterSet;
> +        description = PlacesUIUtils.getDescriptionFromDocument(doc);

ditto

@@ +295,4 @@
>     */  
> +  bookmarkPage: function(aBrowser, aParent = PlacesUtils.unfiledBookmarksFolderId, aShowEditUI = false) {
> +    if (this._bookmarkPageInProgress)
> +      throw new Error("a bookmarkPage operation is already in progress");

I was expecting to enqueue, but in the end this may open a dialog so I guess it may be fine.

@@ +301,5 @@
> +    let whenDone = () => this._bookmarkPageInProgress = false;
> +    return Task.spawn(function() {
> +      // We may set the Places TM to "batching" mode (see below),
> +      // but if anything goes wrong we must undo it right away.
> +      let startedBatching = false;

I'm not sure how this differs from the _batching we are traking in beginBatch, nor what the comment means

@@ +324,5 @@
> +          PlacesUtils.transactionManager.doTransaction(txn);
> +          itemId = txn.item.id;
> +
> +          // Set the character-set.
> +          if (charset && !PrivateBrowsingUtils.isWindowPrivate(aBrowser.contentWindow))

ditto for contentWindow

@@ +325,5 @@
> +          itemId = txn.item.id;
> +
> +          // Set the character-set.
> +          if (charset && !PrivateBrowsingUtils.isWindowPrivate(aBrowser.contentWindow))
> +            yield PlacesUtils.setCharsetForURI(uri, charset);

this change should be a transaction, sigh :(

@@ +347,5 @@
> +          return;
> +        }
> +
> +        let pageProxyFavicon = document.getElementById("page-proxy-favicon");
> +        if (isElementVisible(pageProxyFavicon)) {

nit: I wonder if there's still any case we don't show it, I think all of the browser windows now show the locationbar and we never hide the proxy favicon

@@ +361,2 @@
>  
> +        throw ex;

sounds like would be nicer in the task error handler?

@@ +367,5 @@
>    /**
>     * Adds a bookmark to the page loaded in the current tab. 
>     */
> +  bookmarkCurrentPage: function(aShowEditUI, aParent) 
> +    this.bookmarkPage(gBrowser.selectedBrowser, aParent, aShowEditUI),

let's avoid useless changes to reduce patch and blame, since you are not effectively touching this :)

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The patch is likely obsolete and bitrotted, we should still fix the issues pointed out in comment 0

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Note that removing old transactions will also remove the getPageTitle call, so we really just have to check the reported issues are fixed in the new code.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

looks like there's nothing left to do.