Closed Bug 446171 Opened 16 years ago Closed 8 years ago

Name field of bookmarks saved via "Bookmark All Tabs" has "(null)" value if history is disabled or just in private browsing mode

Categories

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

Product:
Firefox
Component:
Bookmarks & History
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: csbox, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [STR in comment 8])

Attachments

(2 files, 3 obsolete files)

MozReview Request: Bug 446171 - Part 2: Add a browser test for bookmark all tabs; r?mak
58 bytes, text/x-review-board-request
mak
: review+
Details
MozReview Request: Bug 446171 - Part 1: Get page titles from tab and not depend on history; r?mak
58 bytes, text/x-review-board-request
mak
: review+
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.1) Gecko/2008070208 Firefox/3.0.1

If "Bookmarks - Bookmark All Tabs..." is applied to several tabs, the particular bookmarks do not get any page title. Part of the URL is shown in the bookmarks folder instead.

Reproducible: Always

Steps to Reproduce:
1. Open several tabs
2. Open http://www.mozilla.org/community/
3. Select "Bookmarks - Bookmark All Tabs..."
4. Enter a Folder name, e.g., "foo"
5. Select "Bookmarks - foo"
Actual Results:  
In the subfolder "foo" of the bookmark folder, the corresponding bookmark to http://www.mozilla.org/community/ appears not as "Mozilla Community" but as "/community/" instead. If you right-click on "/community/" and select "Properties", the appearing dialogue window shows the title "Properties for "(null)"" and the included name field is empty.

Expected Results:  
I expect Firefox to fill in the page title ("Mozilla Community" in the example) into the name field of each particular bookmark generated by "Bookmark All Tabs...".

about:buildconfig

Build platform
target
i686-pc-mingw32

Build tools
Compiler 	Version 	Compiler flags
cl 	14.00.50727.762 	-GL -wd4624 -wd4952 -TC -nologo -W3 -Gy -Fd$(PDBFILE)
cl 	14.00.50727.762 	-GR- -GL -wd4624 -wd4952 -TP -nologo -Zc:wchar_t- -W3 -Gy -Fd$(PDBFILE)

Configure arguments
--enable-application=browser --enable-update-channel=release --enable-optimize --disable-debug --disable-tests --enable-update-packaging --enable-official-branding --enable-jemalloc --with-crashreporter-enable-percent=25
WFM
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081107
Minefield/3.1b2pre
Status: UNCONFIRMED → RESOLVED
Closed: 16 years ago
Resolution: --- → WORKSFORME
I am experiencing the exact same symptoms as the OP. Initially noticed this problem a couple of versions ago when I first tried this feature. Currently on 3.6.11 (WinXP) and still the same.

User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-GB; rv:1.9.2.11) Gecko/20101012 Firefox/3.6.11 ( .NET CLR 3.5.30729)

I have 100+ tabs open (hence need to bookmark en masse). Tabs have possibly been open for many weeks without being revisited for a while.

Some tabs do bookmark successfully - but these tend to be pages I have visited recently (during the last browser session). The problem can be partially worked around by clearing the cache / browser history and manually visiting each tab and refreshing the page. Simply closing the browser and reopening (after clearing cache) does not help.
Solution. There appears to be a conflict with the "Wired-Marker V3.6.10021200" extension. Disabling this extension (and restarting) appears to resolve this issue for me!

New bookmarks created with "Bookmark All Tabs..." are now saved with the correct title when this extension is disabled.

Even with a set of tabs that had already been bookmarked incorrectly - without a title/name - disabling this extension resulted in the correct title appearing in the Name column of the 'Organise Bookmarks' window. However, the Name field of these bookmarks is still empty, when viewing the details of the bookmark in the lower half of the window (which seemed odd to me). The tabs are still open in the browser window in this case.
Sorry. I do not understand how you can consider this bug report as "works for me". Today, I verified the behaviour under Ubuntu Linux in Firefox 3.6.13 without any add-ons or extensions, see buildconfig below. The observed behaviour is not nice and not as expected! It does not make sense to me, that the properties of a bookmark depend on the way or the circumstances it was created. However, I agree with you, that this need not to be corrected in the short term but definitely in the mid term.

about:buildconfig

Build platform
target
i686-pc-linux-gnu

Build tools
Compiler 	Version 	Compiler flags
gcc 	gcc version 4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5) 	-Wall -W -Wno-unused -Wpointer-arith -Wcast-align -W -Wno-long-long -pedantic -g -fno-strict-aliasing -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions
c++ 	gcc version 4.4.5 (Ubuntu/Linaro 4.4.4-14ubuntu5) 	-fno-rtti -fno-exceptions -Wall -Wpointer-arith -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wcast-align -Wno-invalid-offsetof -Wno-long-long -pedantic -g -fno-strict-aliasing -fshort-wchar -pthread -pipe -DNDEBUG -DTRIMMED -Os -freorder-blocks -fno-reorder-functions

Configure arguments
--build=i686-linux-gnu --prefix=/usr '--includedir=/usr/include' '--mandir=/usr/share/man' '--infodir=/usr/share/info' --sysconfdir=/etc --localstatedir=/var '--libexecdir=/usr/lib/firefox' --disable-maintainer-mode --disable-dependency-tracking --disable-silent-rules --srcdir=. --enable-optimize --enable-ipc --enable-tests --enable-mochitest --disable-system-cairo --disable-system-sqlite --without-system-nspr --without-system-nss --enable-extensions=default --enable-crashreporter --disable-debug --with-user-appdir=.mozilla --without-system-jpeg --without-system-zlib --enable-system-myspell --disable-composer --disable-elf-dynstr-gc --disable-gtktest --disable-install-strip --disable-installer --disable-ldap --disable-mailnews --disable-profilesharing --disable-strip --disable-strip-libs --disable-tests --disable-mochitest --disable-updater --disable-xprint --enable-application=browser --enable-canvas --enable-default-toolkit=cairo-gtk2 --enable-gnomevfs --enable-pango --enable-postscript --enable-svg --enable-mathml --enable-xft --enable-xinerama --enable-safe-browsing --enable-single-profile --with-distribution-id=com.ubuntu --enable-startup-notification --enable-official-branding
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Version: unspecified → 3.6 Branch
Worksforme does not mean that it has to work for you, just that with the steps you reported I could not reproduce the bug, here it saves the correct title for each tab.
The buildconfig is not that interesting, are you saying you can reproduce the bug using the steps in comment 0 in a newly created profile?
Does that happen in both Windows and Linux, or Linux only?
Just ran into this issue today with Firefox 4.0 when doing "Bookmark All Tabs" on some bugzilla pages.
Today, I tried the above steps in Firefox 4 under Windows 7 using a fresh profile. The behaviour was as expected: All bookmarks have been saved with their correct title and not just the URL. Fine. Marco, you have been right. Sorry for my misinterpretation of worksforme.

Using my previous profile, the result was identical to that in comment 0, i.e., there must have been some odd setting in my profile. I do not know, which setting is responsible. I will observe the issue.
I can confirm this bug and have found where the issue lies. It appears to be some kind of conflict with the custom browsing history setting.

I am running Firefox 4.0.1 on Windows XP SP3 32-bit and I can re-create this problem every time using the following steps:

1) Start Firefox using a completely new profile ("firefox -P" is helpful.)

2) Go to Tools->Options->Privacy, select "Use custom settings for history" and deselect "Remember my browsing history." Save the settings.

3) Close the default Firefox "Welcome" tabs, and using the search bar open for example Google and Yahoo in tabs.

4) Select the "Bookmark All Tabs..." from the right-click menu on the tabs and save them in a folder (default is fine.)

5) The bookmarks will have been saved without titles and will only show the URLs instead.

I hope this can help in getting this annoying bug fixed.
Ah, this is possible, we could be trying to get title of the page from history that doesn't have it. confirming to check steps in comment 8
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [STR in comment 8]
OS: Windows XP → All
Hardware: x86 → All
Summary: Name field of bookmarks saved via "Bookmark All Tabs" has "(null)" value → Name field of bookmarks saved via "Bookmark All Tabs" has "(null)" value if history is disabled
Version: 3.6 Branch → Trunk
I can reproduce the steps in comment 8 with the described effect.

This bug is nasty in that it makes searching for one among several bookmarks from the same domain practically impossible since the bookmark titles are practically the same.
I've experienced this bug for several years now also, and can confirm comment #8 is my case locally here - history disabled.
Now, this bug happens in Private window.
So, This bug became more common. I think should be fixed.


Steps To Reproduce.
1. File > New Private Window (Ctrl+Shift+P)
2. Open Several tabs
3. Right click on the tab and Choose "Bookmarks All Tabs..."
4. Click "Add Bookmarks" in dialog

Actual Results:
The bookmarks will have been saved without titles and will only show the URLs instead.

Expected Results:
The bookmarks should be saved with titles
Summary: Name field of bookmarks saved via "Bookmark All Tabs" has "(null)" value if history is disabled → Name field of bookmarks saved via "Bookmark All Tabs" has "(null)" value if history is disabled or just in private browsing mode
I still have the same issue using Firefox 35.0 in both Linux Debian (wheezy) or Windows 7
Attached patch pilot code (obsolete) — — Splinter Review 

    
    

    
  
atlanto, are you interested in working on this bug?
your solution could do, it just needs some tweaks, for add-ons backwards compatibility.
Flags: needinfo?(euthanasia_waltz)

    
    

    
  
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:38.0) Gecko/20100101 Firefox/38.0

Would be cool if someone could fix it. What would break if you just took the code atlanto supplied as a quickfix?
Priority: -- → P3

    
    

    
  
Reader View is also affected as "about:reader?url=" doesn't get recorded in history (Bug 1180551)
Flags: needinfo?(euthanasia_waltz)

    
    

    
  
Hi Marco, I'd be happy to take a look at this bug. I've been working on Firefox OS but have never landed a patch on Firefox desktop. Would you say this is appropriate as a first-bug?

Also, I saw that someone had made a patch, and you suggested tweaking it for "add-ons backwards compatibility", could you elaborate on that? Thanks!
Flags: needinfo?(mak77)

    
    

    
  
(In reply to Scott Wu [:scottwu] from comment #18)
> Hi Marco, I'd be happy to take a look at this bug. I've been working on
> Firefox OS but have never landed a patch on Firefox desktop. Would you say
> this is appropriate as a first-bug?

Well, it's not a strict first-bug (those are usually trivial or very easy) but it's actionable.

> Also, I saw that someone had made a patch, and you suggested tweaking it for
> "add-ons backwards compatibility", could you elaborate on that? Thanks!

Yes, the patch approach may work, that is basically passing also the tab title along with the uri.

The add-ons compatibility problem we need to address is the fact the code is changing bookmarkProperties.js to assume URIList is a list of objects, rather than a list of URIs. Add-ons may still pass a list of uris...  to retain backwards compatibility we need to gently handle both cases, that _URIs is a list of object OR a list of URIs.

Additionally now we have another problem, that is e01s (multiprocess) compatibility. The code in the patch was mostly taken from bookmarkPage (http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-places.js#341) but we had to change that code cause in e10s we cannot access tab.linkedBrowser.contentDocument or tab.linkedBrowser.contentTitle, we need to do something similar to what Neil did for bookmark page
See this checkin http://hg.mozilla.org/mozilla-central/rev/55c331a6f9da

Finally, it would be nice to write a browser-chrome test in browser/components/places/tests/browser, that adds a couple tabs, removes history, bookmarks the tabs and then checks the bookmarks title.
Flags: needinfo?(mak77)

    
    

    
  
Thanks Marco! I'll give it a try and ask you for tips when I (inevitably) run into problems :)
Assignee: nobody → scwwu

    
    

    
  


  
Hi Mak, I got a patch working that doesn't depend on history and works under e10s. Wonder if I could get some feedback from you?

I think I'm supposed to use mozreview but I haven't tried it yet because I'm still working on tests. Or is it okay to put up a WIP patch?

I used the same message listener as the patch by Neil (http://hg.mozilla.org/mozilla-central/rev/55c331a6f9da#l2.12), and added a title attribute.

Thank you!
Flags: needinfo?(mak77)

    
    

    
  
(In reply to Scott Wu [:scottwu] from comment #21)
> I think I'm supposed to use mozreview but I haven't tried it yet because I'm
> still working on tests. Or is it okay to put up a WIP patch?

You can push to mozreview without specifying a reviewer in the commit message and then edit the commit message (hg commit --amend) at a later time when you're ready.
Then you can set the feedback flag in bugzilla on the mozreview attachment.

It's not a perfect solution, but I don't think currently there's an official way to get feedback or a first-pass review on mozreview
Flags: needinfo?(mak77)

    
    

    
  
Comment on attachment 8735412 [details] [diff] [review]
0001-Bug-446171-Get-page-titles-with-browser.messageManag.patch

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

::: browser/base/content/browser-places.js
@@ +550,2 @@
>     */
> +  uniqueCurrentPages: Task.async(function *PCH_uniqueCurrentPages() {

at least this test needs to be fixed if you change this getter
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_visibleTabs_bookmarkAllPages.js#20

We'll have to set addon-compat for these changes since we are changing from sync to async methods.

@@ +554,5 @@
>      let URIs = [];
> +    let pageDetails;
> +    let validTabs = gBrowser.visibleTabs.filter(tab => {
> +      return !/^about:(neterror|certerror|blocked)/.test(
> +        tab.linkedBrowser.currentURI.spec);

I'm not sure this is going to work, we should use isErrorPage from the content message instead and eventually filter later...
Is this something you decided to do additionally? cause doesn't looks like the code was doing that.
The problem is that I may want to bookmark a page even if currently shows some kind of error, cause maybe it's a temporary cert error or such. It's hard to tell.
Or maybe you introduced this filter cause error pages don't have a title...

I fear we may need to keep those urls, and rather change the code in bookmarkProperties to try to fallback to history title when the title is null.

@@ +569,5 @@
>        }
>      });
> +
> +    pageDetails = yield Promise.all(browsers.map(this._getPageDetails))
> +      .catch(err => console.log(err));

I think you may be able to use tab.linkedBrowser.contentTitle and avoid all this complication.
It should be available, worth a try.

::: browser/base/content/content.js
@@ +808,4 @@
>    let isErrorPage = /^about:(neterror|certerror|blocked)/.test(doc.documentURI);
>    sendAsyncMessage("Bookmarks:GetPageDetails:Result",
>                     { isErrorPage: isErrorPage,
> +                     title: PlacesUIUtils.getBestTitle(doc),

I don't think getBestTitle is going to work, it expects a Places node as argument...

But if you use contentTitle you likely don't need this.

::: browser/components/places/content/bookmarkProperties.js
@@ +559,5 @@
>      for (let uri of this._URIs) {
> +      // Bug 446171 fixes 'Bookmark All Tabs' bug by changing uri into
> +      // an object that includes page title. We check if uri is indeed an object.
> +      // If not, we fallback to the original implementation, just in case any addon
> +      // depends on it bring a string.

There is usually no need to specify the bug in comments, since we have blame information that point to it. So here I'd limit the comment to:

// uri should be an object in the form { url, title }. Though add-ons
// could still use the legacy form, where it's a plain uri.

@@ +566,5 @@
> +                                            PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                            uri.title) :
> +        new PlacesCreateBookmarkTransaction(uri, -1,
> +                                            PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                            this._getURITitleFromHistory(uri));

let title = typeof uri == "object" ?
  uri.title : this._getURITitleFromHistory(uri);

let createTxn = ...

    
    

    
  
Comment on attachment 8735412 [details] [diff] [review]
0001-Bug-446171-Get-page-titles-with-browser.messageManag.patch

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

Thank you so much for the detailed feedback! I really appreciate it :)

::: browser/base/content/browser-places.js
@@ +550,2 @@
>     */
> +  uniqueCurrentPages: Task.async(function *PCH_uniqueCurrentPages() {

Ok I'll change the test from a getter to a function call.

I'm not familiar with what you meant by set 'addon-compat', could you elaborate on that?

@@ +554,5 @@
>      let URIs = [];
> +    let pageDetails;
> +    let validTabs = gBrowser.visibleTabs.filter(tab => {
> +      return !/^about:(neterror|certerror|blocked)/.test(
> +        tab.linkedBrowser.currentURI.spec);

Yes you are right this should be changed. I originally thought the URLs for error pages would become something like "about:blocked?e=...&u=..." and it wouldn't be useful to save them. I must have mistaken it with documentURI rather than currentURI.

Title was another reason I opted to filter it out, but I see the original behaviour is to just use the URL, which makes sense too.

@@ +569,5 @@
>        }
>      });
> +
> +    pageDetails = yield Promise.all(browsers.map(this._getPageDetails))
> +      .catch(err => console.log(err));

tab.linkedBrowser.contentTitle was the first thing I tried, but I couldn't get it to work under e10s. I'll give it a try again to confirm.

::: browser/components/places/content/bookmarkProperties.js
@@ +559,5 @@
>      for (let uri of this._URIs) {
> +      // Bug 446171 fixes 'Bookmark All Tabs' bug by changing uri into
> +      // an object that includes page title. We check if uri is indeed an object.
> +      // If not, we fallback to the original implementation, just in case any addon
> +      // depends on it bring a string.

Got it. Thanks!

@@ +566,5 @@
> +                                            PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                            uri.title) :
> +        new PlacesCreateBookmarkTransaction(uri, -1,
> +                                            PlacesUtils.bookmarks.DEFAULT_INDEX,
> +                                            this._getURITitleFromHistory(uri));

Thanks, this is more concise.

    
    

    
  
Hi Marco,

Hope I'm using MozReview correctly here. It turns out you were right, I could just use tab.linkedBrowser.contentTitle to get the page title. Not sure why it didn't work for me the first time I tried it.. I've got rid off some code and now the diff is much simpler.

I've also added a test to check if the titles and URLs are returned correctly. Initially I tried to emulate actually clicking and creating the bookmarks through browser UI, and I got to the point where I created tabs and clicked on 'Bookmark All Tabs', but I couldn't figure out how to enter folder name and click 'Add Bookmarks' in the dialog box. Please let me know if you know how it could be done, or references that might have the solution to that.

Thanks a lot again!
Flags: needinfo?(mak77)

        Attachment #8735412 -
        Attachment is obsolete: true
Flags: needinfo?(mak77)

        Attachment #8600514 -
        Attachment is obsolete: true

        Attachment #8741567 -
        Flags: review?(mak77) → review+

    
    

    
  
Comment on attachment 8741567 [details]
MozReview Request: Bug 446171 - Part 2: Add a browser test for bookmark all tabs; r?mak

https://reviewboard.mozilla.org/r/46571/#review43349

This looks fine, clearly we'll need a Try run to be sure there's no other test breaking... Do you have access to the Try server?

::: browser/base/content/browser-places.js:560
(Diff revision 1)
> +      let browser = tab.linkedBrowser;
> +      let URI = browser.currentURI;
> +      let spec = URI.spec;
>        if (!tab.pinned && !(spec in uniquePages)) {
>          uniquePages[spec] = null;
> -        URIs.push(tab.linkedBrowser.currentURI);
> +        URIs.push({ uri: URI, title: browser.contentTitle });

nit: if you name the variable "uri" (lowercase) then here you can use an ES 2015 property shorthand.
Practically instead of { myproperty: myproperty } you can just write { myproperty }
in this case { uri, title: browser.contentTitle }

See https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015

    
    

    
  
Comment on attachment 8741568 [details]
MozReview Request: Bug 446171 - Part 2: Add a browser test for bookmark all tabs; r?mak

https://reviewboard.mozilla.org/r/46573/#review43359

::: browser/base/content/test/general/browser.ini:24
(Diff revision 1)
>    browser_star_hsts.sjs
>    browser_tab_dragdrop2_frame1.xul
>    browser_web_channel.html
>    browser_web_channel_iframe.html
> +  bookmark_dummy_1.html
> +  bookmark_dummy_2.html

why are these in borwser/base/content, if the test is in browser/components/places/tests/browser?
Just move these there and update the related browser.ini

::: browser/components/places/tests/browser/browser_bookmark_all_tabs.js:6
(Diff revision 1)
> +"use strict"
> +
> +add_task(function* () {
> +  info("Bug 446171 - Name field of bookmarks saved via 'Bookmark All Tabs' " +
> +       "has '(null)' value if history is disabled or just in private " +
> +       "browsing mode");

just make this a comment, we don't need tests to be too much descriptive in their output, since it's mostly consumed by machines.
I prefer to put info where it can help figuring out why we see a failure.
The test description usually is in the first lines of the test file as a comment. in case of test with multiple subtests, then we use to info a brief title for the running test, just to check in the log where we arrived in the test.

::: browser/components/places/tests/browser/browser_bookmark_all_tabs.js:11
(Diff revision 1)
> +       "browsing mode");
> +
> +  const TEST_PAGES = [
> +    "http://example.org/browser/browser/base/content/test/general/bookmark_dummy_1.html",
> +    "http://example.org/browser/browser/base/content/test/general/bookmark_dummy_2.html",
> +    "http://example.org/browser/browser/base/content/test/general/bookmark_dummy_1.html"

nit: You can assign the base url to a const and shorten these.

::: browser/components/places/tests/browser/browser_bookmark_all_tabs.js:14
(Diff revision 1)
> +    "http://example.org/browser/browser/base/content/test/general/bookmark_dummy_1.html",
> +    "http://example.org/browser/browser/base/content/test/general/bookmark_dummy_2.html",
> +    "http://example.org/browser/browser/base/content/test/general/bookmark_dummy_1.html"
> +  ];
> +
> +  function promiseAddTab(url) {

We use common utils to load tabs and such cause custom methods very often can cause random failures when we are in e10s multi-process mode.

Take a look at the utils in http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm (openForegroundTab and so on)

BrowserTestUtils should already by in context for these tests, cause it's being imported by head.js

::: browser/components/places/tests/browser/browser_bookmark_all_tabs.js:39
(Diff revision 1)
> +  Assert.deepEqual(URIs.map(URI => URI.title), [
> +    "", "Bookmark Dummy 1", "Bookmark Dummy 2"
> +  ], "Correct titles are returned");
> +
> +  registerCleanupFunction(function* () {
> +    tabs.forEach(tab => gBrowser.removeTab(tab));

ditto, should use BrowserTestUtils.removeTab

    
    

    
  
https://reviewboard.mozilla.org/r/46571/#review43349

Great! Yes I got level 1 access already, but I'm looking at http://trychooser.pub.build.mozilla.org/ and not sure what options to set.. "Both" build types, "All" platforms, and "All" unit test suites?

> nit: if you name the variable "uri" (lowercase) then here you can use an ES 2015 property shorthand.
> Practically instead of { myproperty: myproperty } you can just write { myproperty }
> in this case { uri, title: browser.contentTitle }
> 
> See https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015

Thanks! I'll use the new syntax then.

    
    

    
  
(In reply to Scott Wu [:scottwu] from comment #30)
> https://reviewboard.mozilla.org/r/46571/#review43349
> 
> Great! Yes I got level 1 access already, but I'm looking at
> http://trychooser.pub.build.mozilla.org/ and not sure what options to set..
> "Both" build types, "All" platforms, and "All" unit test suites?

Usually for a patch affecting desktop Firefox you can use this
try: -b do -p linux,linux64,macosx64,win32,win64 -u xpcshell,marionette,marionette-e10s,mochitests -t none

so both, on desktop platforms, xpcshell, marionette and mochitests.

    
    

    
  
Thanks! Here's the result from try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d5ced4998b

Lot's of orange but none seem to be related to this patch. Wonder if the orange I see is normal?

    
    

    
  
Hi Marco, I've fixed the issues you pointed out in part 2. Please let me know if it looks okay now.
Thanks a lot!
https://reviewboard.mozilla.org/r/46573/#review43359
Flags: needinfo?(mak77)

    
    

    
  
(In reply to Scott Wu [:scottwu] from comment #32)
> Thanks! Here's the result from try:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f4d5ced4998b
> 
> Lot's of orange but none seem to be related to this patch. Wonder if the
> orange I see is normal?

that orange is a lot, usually intermittent failures only happen for a single platform, or for a single run (they are automatically retriggered). In your push there are a lot of failures that don't look intermittent. For example mochitest 3 and 5 looks suspicious.
Note, it's possible you rebased on top of a changeset who cause these failures. Thus I'd suggest you rebase on top of a changeset that is mostly green on the original tree you are working on (may be inbound or fx-team...) so you are sure to not inherit failures from others.
Flags: needinfo?(mak77)

    
    

    
  
sorry, I don't see the patch changes, it still looks like before. Are you sure you published the review request, and it's not still in draft state?
Flags: needinfo?(scwwu)

    
    

    
  
Comment on attachment 8741567 [details]
MozReview Request: Bug 446171 - Part 2: Add a browser test for bookmark all tabs; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46571/diff/1-2/

    
  

        Attachment #8741568 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8741567 [details]
MozReview Request: Bug 446171 - Part 2: Add a browser test for bookmark all tabs; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46571/diff/2-3/

        Attachment #8741567 -
        Attachment description: MozReview Request: Bug 446171 - Part 1: Get page titles from tab and not depend on history; r?mak → MozReview Request: Bug 446171 - Part 2: Add a browser test for bookmark all tabs; r?mak

    
    

    
  
Sorry think I messed up the review commits.. and I'll rebase and try again to see if the oranges goes away. Thanks again.
Flags: needinfo?(scwwu)

    
    

    
  
Comment on attachment 8745817 [details]
MozReview Request: Bug 446171 - Part 1: Get page titles from tab and not depend on history; r?mak

https://reviewboard.mozilla.org/r/49105/#review46071

r=me with the following fixed and a green-ish Try run.

::: browser/components/places/content/bookmarkProperties.js:562
(Diff revision 1)
>    _getTransactionsForURIList: function BPP__getTransactionsForURIList() {
>      var transactions = [];
>      for (let uri of this._URIs) {
> -      let title = this._getURITitleFromHistory(uri);
> -      let createTxn = new PlacesCreateBookmarkTransaction(uri, -1,
> +      // uri should be an object in the form { url, title }. Though add-ons
> +      // could still use the legacy form, where it's a plain uri.
> +      let [_uri, _title] = typeof uri == 'object' ?

Unfortunately this test is not enough, indeed if you do
typeof makeURI("http://localhost") you get "object".

instead let's invert the condition

uri instanceof Ci.nsIURI ?
  [uri, this._getURITitleFromHistory(uri)] : [uri.uri, uri.title];
  
Also, please change the comment from "a plain uri" to "an nsIURI"

        Attachment #8745817 -
        Flags: review?(mak77) → review+

    
    

    
  
Comment on attachment 8745817 [details]
MozReview Request: Bug 446171 - Part 1: Get page titles from tab and not depend on history; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/49105/diff/1-2/

    
    

    
  
Comment on attachment 8741567 [details]
MozReview Request: Bug 446171 - Part 2: Add a browser test for bookmark all tabs; r?mak

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46571/diff/3-4/

    
    

    
  
https://reviewboard.mozilla.org/r/49105/#review46071

Here's a new try with mostly greens and some oranges, most of which became green on the 2nd try. The oranges seem to be platform specific:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2395b3eecd6

Wonder what you think of it? Thanks!

> Unfortunately this test is not enough, indeed if you do
> typeof makeURI("http://localhost") you get "object".
> 
> instead let's invert the condition
> 
> uri instanceof Ci.nsIURI ?
>   [uri, this._getURITitleFromHistory(uri)] : [uri.uri, uri.title];
>   
> Also, please change the comment from "a plain uri" to "an nsIURI"

Yes, thank you for catching this. I've made the change.

    
    

    
  
the Try looks good, all the failures have a green retrigger, but bc4 that is unlikely related to this change.
I think you can set the checkin-needed keyword.
Thank you!

    
    

    
  
Great! Thank you so much for all the help Marco :)
Keywords: checkin-needed

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/1affe99093f5
https://hg.mozilla.org/mozilla-central/rev/641ef4f3b953
Status: NEW → RESOLVED
Closed: 16 years ago → 8 years ago

          status-firefox49:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49

    
    

    
  
This doesn't work for unloaded session tabs (Bug 754623).

    
    

    
  
I have  reproduced this bug with Firefox(minefield) 3.1a1 (build id:2008070203)on
windows 7(64 bit)

I have verified this bug as fixed with Firefox beta 49.0b7(build id:20160825132718)
User Agent:Mozilla/5.0 (Windows NT 6.1; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0

[testday-20160826]

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: