737046 - Places protocol handler does not work.Remove it.

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Philip Chee]

STR:
Type (or paste) the following into the location bar:
place:folder=BOOKMARKS_MENU&folder=UNFILED_BOOKMARKS&folder=TOOLBAR&sort=12&excludeQueries=1&queryType=1

Actual result:

File not found
Firefox can't find the file at jar:file:///C:/Program Files/Mozilla.Org/Minefield/omni.ja!/chrome/browser/content/browser/places/content-ui/controller.xhtml.

  Check the file name for capitalization or other typing errors.
  Check to see if the file was moved, renamed or deleted.

Expected result:

Some sort of formatted page displaying bookmarks.

Either remove the protocol handler or reimplement chrome://browser/content/places/content-ui/controller.xhtml

http://hg.mozilla.org/mozilla-central/annotate/58a2cd0203ee/browser/components/places/src/PlacesProtocolHandler.js#l47

It's not a regression in the sense that I don't think it actually worked at all as far as I can tell.

Comment 1

[Philip Chee]

Apparently the UI was being worked on in Bug 529605 (Places data should be visible in content) but was eventually abandoned.

Comment 2

[Marco Bonardo [:mak]]

it was a first attempt that was never finished, the feature is still planned though the protocol is basically dead atm.

Comment 3

[Philip Chee]

At the moment the protocol handler doesn't do anything useful and is just code bloat. Since Bug 529605 is WONTFIX, perhaps the placesProtocolHandler should be removed from the tree. Otherwise it just gives more useless work to people trying to keep the code working and up to date e.g. Bug 568691.

If Bug 529605 is ever un-WONTFIXed then it can always be checked in again.

Comment 4

[Marco Bonardo [:mak]]

Since it's unmaintained, which kind of additional work does it give? That code doesn't change from months (maybe even year?) and being in browser the backport can be avoided if needed.
Btw, if you want to make a patch to remove it, I will review it.

Comment 5

[Renato Silva]

I've got the place handler working by creating a bookmark in the location bar, then moving to bookmarks menu, then moving back to location bar.

Comment 6

[Marco Bonardo [:mak]]

(In reply to Renato Silva from comment #5)
> I've got the place handler working by creating a bookmark in the location
> bar, then moving to bookmarks menu, then moving back to location bar.

that's unrelated, the place: "protocol" works for queries, this handler was created to be able to show queries in a content page.

this is what I found in mxr and should be removed

http://mxr.mozilla.org/mozilla-central/search?string=PlacesProtocolHandler

more specifically:

/b2g/installer/package-manifest.in should be updated

BrowserPlaces.manifest should be removed and thus its reference should be removed from:
/b2g/installer/package-manifest.in 
/browser/components/places/moz.build
/browser/installer/package-manifest.in

/browser/components/places/moz.build should be updated

/browser/installer/package-manifest.in should be updated

browser/components/places/src/PlacesProtocolHandler.js should be removed

Comment 7

[Abhishek Bhattacharya [:babhishek21]]

Hi. I would like to work on this bug. New to FOSS, so might need some help/mentoring. 

Seems pretty straightforward. Shall I start on a patch?

Comment 8

[Abhishek Bhattacharya [:babhishek21]]

Attachment: bug_737046

Removed browser/components/places/PlacesProtocolHandler.js
Removed browser/components/places/BrowserPlaces.manifest

Updated /b2g/installer/package-manifest.in 
Updated /browser/components/places/moz.build
Updated /browser/installer/package-manifest.in

Please review.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 8547554 [details] [diff] [review]
bug_737046

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

thank you for the patch, there are still some changes to do though.

You didn't remove all the occurences of BrowserPlaces.manifest, see
http://mxr.mozilla.org/mozilla-central/search?string=BrowserPlaces.manifest

::: browser/components/places/moz.build
@@ +9,5 @@
>  BROWSER_CHROME_MANIFESTS += ['tests/browser/browser.ini']
>  
>  JAR_MANIFESTS += ['jar.mn']
>  
> +EXTRA_COMPONENTS += []

you should remove this line, there's no point in passing an empty array.

Comment 10

[Abhishek Bhattacharya [:babhishek21]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #9)
> Comment on attachment 8547554 [details] [diff] [review]
> bug_737046
> 
> Review of attachment 8547554 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> thank you for the patch, there are still some changes to do though.
> 
> You didn't remove all the occurences of BrowserPlaces.manifest, see
> http://mxr.mozilla.org/mozilla-central/search?string=BrowserPlaces.manifest
Hmm. I missed that. Sorry.
> ::: browser/components/places/moz.build
> @@ +9,5 @@
> >  BROWSER_CHROME_MANIFESTS += ['tests/browser/browser.ini']
> >  
> >  JAR_MANIFESTS += ['jar.mn']
> >  
> > +EXTRA_COMPONENTS += []
> 
> you should remove this line, there's no point in passing an empty array.

Haha. I was expecting that pickle. I just left it in case some other additional components required to be passed. Will work on it , Cap'n.

Comment 11

[Abhishek Bhattacharya [:babhishek21]]

Attachment: bug_737046_1: rev_1 (Minor changes)

Fixed empty array in /browser/components/places/moz.build
Removed BrowserPlaces.manifest refs from:
1. /b2g/installer/package-manifest.in
2. /browser/installer/package-manifest.in

Review please.

Comment 12

[Marco Bonardo [:mak]]

Comment on attachment 8548054 [details] [diff] [review]
bug_737046_1: rev_1 (Minor changes)

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

Thank you.

In future you can ask for review by setting the review flag on a patch, very often people wouldn't notice a review request in a plain comment cause we get hundreds mails a day.

Now, I will push your patch to the Try server to run automated tests.

Comment 13

[Marco Bonardo [:mak]]

ah I forgot one thing, please update the commit message to be in the form:

Bug 737046 - Remove dead Places protocol handler. r=mak

Comment 14

[Marco Bonardo [:mak]]

and while there, looks like something just landed that bitrotted the patch in package-manifest.in, please pull latest source and update the patch.

Comment 15

[Abhishek Bhattacharya [:babhishek21]]

Attachment: bug_737046_2: rev_1 (Updated Commit message to match standards)

Updated commit message to match standards.

Comment 16

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=2926218a4e1c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1d2e2c1bd171

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1d2e2c1bd171

Comment 19

[Abhishek Bhattacharya [:babhishek21]]

What next?

Comment 20

[Marco Bonardo [:mak]]

The work on this bug is complete, someone from QA will soon or later verify the bug has been fixed.
If you wish, you can start working on a new bug
http://www.joshmatthews.net/bugsahoy/?unowned=1&simple=1
when you feel you are comfortable enough with the codebase you can remove the "simple bugs" flag from there and look at more complex mentored bugs.

Comment 21

[Marco Bonardo [:mak]]

You might also take a look at the list of bugs I mentor, some are still unassigned or are blocked from a long time and you could help completing them:

https://bugzilla.mozilla.org/buglist.cgi?list_id=11863400&resolution=---&query_based_on=mentored&emailtype1=exact&query_format=advanced&email1=mak77%40bonardo.net&emailbug_mentor1=1&known_name=mentored

Comment 22

[Abhishek Bhattacharya [:babhishek21]]

(In reply to Marco Bonardo [::mak] (needinfo? me) from comment #21)
> You might also take a look at the list of bugs I mentor, some are still
> unassigned or are blocked from a long time and you could help completing
> them:
> 
> https://bugzilla.mozilla.org/buglist.cgi?list_id=11863400&resolution=---
> &query_based_on=mentored&emailtype1=exact&query_format=advanced&email1=mak77%
> 40bonardo.net&emailbug_mentor1=1&known_name=mentored

Thanks for the link. I have one question. I use Mercurial queues to generate patches. Can I change this patch into a permanent changeset? Will it interfere with any further pulls that I make?

Comment 23

[Marco Bonardo [:mak]]

(In reply to Abhishek Bhattacharya from comment #22)
> Thanks for the link. I have one question. I use Mercurial queues to generate
> patches. Can I change this patch into a permanent changeset? Will it
> interfere with any further pulls that I make?

you can, but it's not worth it, you might in some cases end up with conflicts when you pull, and you'll have to manage rebasing.
I'd just hg qdelete the patch, and pull the most recent source. What I used to do was popping all of the patch (hg qpop -a), updating (hg pull -u), then reapplying the patches I cared about (hg qpush, after hg qdelete the no more relevant ones)