Add the type of storage in dom-storage2-change notification

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Optimizer, Assigned: Optimizer)

Tracking

({dev-doc-needed})

unspecified
mozilla30
dev-doc-needed
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [storage])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 8373411 [details] [diff] [review]
dom-storage2-change

Currently, the only way to detect the type of storage change (ie. local storage or session storage) is to do subject.storageArea == window.(local|session)Storage in the notification observer of "dom-storage2-change"

This assumes that you have a reference to the window object handy.

For a storage devtool (bug 965872) there are cases when even though we have window object with us, and it is the correct window object for the notification, window.(local|session)Storage == subject.storageArea is never true. This is perhaps because the window object was obtained via the "load" event of the window's docshell's ChromeEventHandler and is somehow internally different than the actual window.

Thus, this patch converts an otherwise null third argument in notification observers to the string representing the type of storage, ie. either "localStorage" or "sessionStorage"

I was not sure if I broke anything else or not, so here goes a try : https://tbpl.mozilla.org/?tree=Try&rev=2c2f3bdf0d10
Attachment #8373411 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Assignee: nobody → scrapmachines
Status: NEW → ASSIGNED

Comment 1

5 years ago
Comment on attachment 8373411 [details] [diff] [review]
dom-storage2-change

Based on jcranmer const char16_t* mType; is safe in this case.

Could you add some test for this, so that we don't accidentally break this later.
Attachment #8373411 - Flags: review?(bugs) → review+
(Assignee)

Comment 2

5 years ago
Created attachment 8374047 [details] [diff] [review]
patch with tests

Added a mochitest plain test which triggers dom-storage2-changed notifications and confirms the type.

try : https://tbpl.mozilla.org/?tree=Try&rev=6b32888b1790
Attachment #8373411 - Attachment is obsolete: true
Attachment #8374047 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Whiteboard: [storage]

Updated

5 years ago
Attachment #8374047 - Flags: review?(bugs) → review+
(Assignee)

Comment 3

5 years ago
landed in fx-team : https://hg.mozilla.org/integration/fx-team/rev/858730718af2
Whiteboard: [storage] → [storage][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/858730718af2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [storage][fixed-in-fx-team] → [storage]
Target Milestone: --- → mozilla30
(Assignee)

Comment 5

5 years ago
I was thinking of editing the mdn doc and adding this additional information which is available from Firefox 30, but there was no mdn doc for this notification at all. Thus the keyword.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.