Closed Bug 1275160 Opened 4 years ago Closed 4 years ago

Web Manifest: don't special case orientation

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: marcosc, Assigned: marcosc)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

The spec recently changed to no longer require orientation to return an empty string. The manifest processor + tests needs to be updated to match.
Blocks: webmanifest
Attachment #8755699 - Flags: review?(mconley)
Assignee: nobody → mcaceres
Comment on attachment 8755699 [details] [diff] [review]
tiny change to how orientation member is processed

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

::: dom/manifest/ManifestProcessor.jsm
@@ +149,3 @@
>        if (value && typeof value === "string" && this.orientationTypes.has(value.toLowerCase())) {
>          return value.toLowerCase();
>        }

This might sound pedantic, but I think I'd prefer if we explicitly return undefined here if that's expected. I suspect that's something a linter would pick up too.
Attachment #8755699 - Flags: review?(mconley) → review+
Whiteboard: btpp-active
(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #3)
> This might sound pedantic, but I think I'd prefer if we explicitly return
> undefined here if that's expected.

No, that's totally reasonable. I should have kept it in from the original. 

> I suspect that's something a linter would
> pick up too.

Heh, I think something like JSHint would complain for the opposite reason: returning undefined is redundant :) However, I also agree that it's clearer. 

Thanks for the speedy review!
Carrying over r+
Attachment #8755699 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1b8749c914b0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.