StringListRange (in dom/media/VideoUtils.h) should have options on how to handle empty items

RESOLVED FIXED in Firefox 53

Status

()

Core
Audio/Video: Playback
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: gerald, Assigned: gerald)

Tracking

49 Branch
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
Currently StringListRange always skips empty items. So "a,,b" will only go through 'a' and 'b', but not the empty element.

For some upcoming work, handling empty items will be important, so I will add an option to do that.

Special consideration is needed for a totally-empty string "": Is it an empty list, or a list of one empty item? Different options will be offered for both situations.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Blocks: 1329568

Comment 3

11 months ago
mozreview-review
Comment on attachment 8824871 [details]
Bug 1329564 - Option to process empty items in StringListRange -

https://reviewboard.mozilla.org/r/103186/#review103860

::: dom/media/VideoUtils.h:369
(Diff revision 1)
>  
> -template <typename String>
> +enum class StringListRangeEmptyItems
> +{
> +  Skip, // Skip all empty items (empty string will process nothing)
> +  ProcessAll, // Process all, including 1 empty item in an empty string
> +  ProcessEmptyItems // Process all, except if string is empty

I don't follow the name or comment here.

So it process Empty Items except if string is empty???
Attachment #8824871 - Flags: review?(jyavenard) → review+

Comment 4

11 months ago
mozreview-review
Comment on attachment 8824872 [details]
Bug 1329564 - StringListRange::begin/end() can be const -

https://reviewboard.mozilla.org/r/103188/#review103864
Attachment #8824872 - Flags: review?(jyavenard) → review+
(Assignee)

Comment 5

11 months ago
mozreview-review-reply
Comment on attachment 8824871 [details]
Bug 1329564 - Option to process empty items in StringListRange -

https://reviewboard.mozilla.org/r/103186/#review103860

> I don't follow the name or comment here.
> 
> So it process Empty Items except if string is empty???

I don't follow your question! You seem to understand, by repeating what the comment says. :-)

The two will process empty items, e.g.: "a,,b" will give "a", "", "b". And "," will give "", "" (two empty items).
The main difference is how they handle an empty string, because users may have different expectations:
- ProcessAll: "" -> one item, which is just ""
- ProcessEmptyItems: "" -> Zero items, nothing will happen.

How could I phrase it better, so it's as clear as possible? I probably could give an example, as above.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 10

11 months ago
Pushed by gsquelart@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/195a58e1ad5e
Option to process empty items in StringListRange - r=jya
https://hg.mozilla.org/integration/autoland/rev/d3da2d43ac5d
StringListRange::begin/end() can be const - r=jya

Comment 11

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/195a58e1ad5e
https://hg.mozilla.org/mozilla-central/rev/d3da2d43ac5d
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.