663269 - Handle null livemarks siteURI

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

siteURI can be null, but we don't handle that, this patch makes so that if siteURI is null we fallback to feedURI, that can never be null.

Sorry for lots of lm bugs, but I'm splitting my livemark refactoring patch in multiple bugs, the main patch will be big enough by itself.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 538383 [details] [diff] [review]
patch v1.0

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

r=me

::: toolkit/components/places/PlacesUtils.jsm
@@ +597,5 @@
>          function gatherDataUrl(bNode) {
>            if (PlacesUtils.nodeIsLivemarkContainer(bNode)) {
> +            let uri = PlacesUtils.livemarks.getSiteURI(bNode.itemId) ||
> +                      PlacesUtils.livemarks.getFeedURI(bNode.itemId);
> +            return uri.spec + NEWLINE + bNode.title;

hm, don't both of these throw? should we handle that in each of these cases in this patch?

Comment 3

[Marco Bonardo [:mak]]

(In reply to comment #2)

> ::: toolkit/components/places/PlacesUtils.jsm
> @@ +597,5 @@
> >          function gatherDataUrl(bNode) {
> >            if (PlacesUtils.nodeIsLivemarkContainer(bNode)) {
> > +            let uri = PlacesUtils.livemarks.getSiteURI(bNode.itemId) ||
> > +                      PlacesUtils.livemarks.getFeedURI(bNode.itemId);
> > +            return uri.spec + NEWLINE + bNode.title;
> 
> hm, don't both of these throw? should we handle that in each of these cases
> in this patch?

Well if it's a livemark it must have a feedURI, and may have an optional siteURI. So the former won't throw, the latter may, but it's unexpected so it's fine to throw.

Comment 4

[Marco Bonardo [:mak]]

http://hg.mozilla.org/projects/places/rev/2075b349013e

Comment 5

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/2075b349013e