Move the exception logic to the AudioNode creator

third_party/WebKit/Source/modules/webaudio/ScriptProcessorNode.cpp

 /*
  * Copyright (C) 2010, Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1.  Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2.  Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 241 matching lines @@
     size_t bufferSize = 1 << static_cast<unsigned>(log2(4 * hardwareBufferSize) + 0.5);
 
     if (bufferSize < 256)
         return 256;
     if (bufferSize > 16384)
         return 16384;
 
     return bufferSize;
 }
 
-ScriptProcessorNode* ScriptProcessorNode::create(AbstractAudioContext& context, float sampleRate, size_t bufferSize, unsigned numberOfInputChannels, unsigned numberOfOutputChannels)
+ScriptProcessorNode* ScriptProcessorNode::create(
+    AbstractAudioContext& context,
+    ExceptionState& exceptionState)
 {
+    ASSERT(isMainThread());

[hongchan, 2016/05/13 01:20:13]

DCHECK.

[Raymond Toy, 2016/05/20 23:12:01]

Done.

+
+    return create(context, 0, 2, 2, exceptionState);
+}
+
+ScriptProcessorNode* ScriptProcessorNode::create(
+    AbstractAudioContext& context,
+    size_t bufferSize,
+    ExceptionState& exceptionState)
+{
+    ASSERT(isMainThread());

[hongchan, 2016/05/13 01:20:13]

Ditto.

[Raymond Toy, 2016/05/20 23:12:01]

Done.

+
+    return create(context, bufferSize, 2, 2, exceptionState);
+}
+
+ScriptProcessorNode* ScriptProcessorNode::create(
+    AbstractAudioContext& context,
+    size_t bufferSize,
+    unsigned numberOfInputChannels,
+    ExceptionState& exceptionState)
+{
+    ASSERT(isMainThread());

[hongchan, 2016/05/13 01:20:13]

Ditto.

[Raymond Toy, 2016/05/20 23:12:01]

Done.

+
+    return create(context, bufferSize, numberOfInputChannels, 2, exceptionState);
+}
+
+ScriptProcessorNode* ScriptProcessorNode::create(
+    AbstractAudioContext& context,
+    size_t bufferSize,
+    unsigned numberOfInputChannels,
+    unsigned numberOfOutputChannels,
+    ExceptionState& exceptionState)
+{
+    ASSERT(isMainThread());
+
+    if (context.isContextClosed()) {
+        context.throwExceptionForClosedState(exceptionState);
+        return nullptr;
+    }
+
+    if (numberOfInputChannels == 0 && numberOfOutputChannels == 0) {
+        exceptionState.throwDOMException(
+            IndexSizeError,
+            "number of input channels and output channels cannot both be zero.");

[hongchan, 2016/05/13 01:20:13]

input channels and output channels => input and output channels

Then we can contain this within 80 cols.

[Raymond Toy, 2016/05/13 16:36:01]

I chose this originally to kind of match the text in the other exceptions below.

Plus, we shouldn't shorten a user message just to make source code fit in 80
cols.  That's the wrong approach.

+        return nullptr;
+    }
+
+    if (numberOfInputChannels > AbstractAudioContext::maxNumberOfChannels()) {
+        exceptionState.throwDOMException(
+            IndexSizeError,
+            "number of input channels (" + String::number(numberOfInputChannels)
+            + ") exceeds maximum ("
+            + String::number(AbstractAudioContext::maxNumberOfChannels()) + ").");
+        return nullptr;
+    }
+
+    if (numberOfOutputChannels > AbstractAudioContext::maxNumberOfChannels()) {
+        exceptionState.throwDOMException(
+            IndexSizeError,
+            "number of output channels (" + String::number(numberOfInputChannels)
+            + ") exceeds maximum ("
+            + String::number(AbstractAudioContext::maxNumberOfChannels()) + ").");
+        return nullptr;
+    }
+

[hongchan, 2016/05/13 01:20:13]

In the change above, we have few "80-col" issues. Perhaps we should discuss this
in person.

     // Check for valid buffer size.
     switch (bufferSize) {
     case 0:
         bufferSize = chooseBufferSize();
         break;
     case 256:
     case 512:
     case 1024:
     case 2048:
     case 4096:
     case 8192:
     case 16384:
         break;
     default:
+        exceptionState.throwDOMException(
+            IndexSizeError,
+            "buffer size (" + String::number(bufferSize)
+            + ") must be 0 or a power of two between 256 and 16384.");
         return nullptr;
     }
 
-    if (!numberOfInputChannels && !numberOfOutputChannels)
-        return nullptr;
+    ScriptProcessorNode* node =  new ScriptProcessorNode(context, context.sampleRate(), bufferSize, numberOfInputChannels, numberOfOutputChannels);

[hongchan, 2016/05/13 01:20:13]

Let's wrap arguments.

 
-    if (numberOfInputChannels > AbstractAudioContext::maxNumberOfChannels())
-        return nullptr;
+    if (node) {
+        // context keeps reference until we stop making javascript rendering callbacks

[hongchan, 2016/05/13 01:20:13]

If we move this comment above |if (node)| Then we can remove curly braces.

[hongchan, 2016/05/20 21:28:32]

Perhaps it needs one more look here?

[Raymond Toy, 2016/05/20 21:46:09]

I like the braces; moving the comment out just so I can remove braces seems
unnecessary.

(FWIW, I hate the rule that braces are not needed.  It's a needless extra rule
and it's a great source of silly bugs when modifying code.)

[hongchan, 2016/05/20 22:57:48]

It goes both ways. Without braces, we can easily tell this is a single action
based on the condition. Also the condition is the part of 'comment' here. Isn't
it?

Anyhow, I will leave this up to you.

+        context.notifySourceNodeStartedProcessing(node);
+    }
 
-    if (numberOfOutputChannels > AbstractAudioContext::maxNumberOfChannels())
-        return nullptr;
-
-    return new ScriptProcessorNode(context, sampleRate, bufferSize, numberOfInputChannels, numberOfOutputChannels);
+    return node;

[hongchan, 2016/05/13 01:20:13]

The same issue before - we should return node when it is valid. Otherwise we
should return nullptr.

[hongchan, 2016/05/20 21:28:32]

Is this addressed somewhere?

[Raymond Toy, 2016/05/20 21:46:09]

If line 352 fails to create a ScriptProcessor, node is null, and we'll return a
null here.

[hongchan, 2016/05/20 22:57:48]

But can we be explicit about that? We can easily return null in the if clause
above.

 }
 
 size_t ScriptProcessorNode::bufferSize() const
 {
     return static_cast<ScriptProcessorHandler&>(handler()).bufferSize();
 }
 
 bool ScriptProcessorNode::hasPendingActivity() const
 {
     // To prevent the node from leaking after the context is closed.
     if (context()->isContextClosed())
         return false;
 
     // If |onaudioprocess| event handler is defined, the node should not be
     // GCed even if it is out of scope.
     if (hasEventListeners(EventTypeNames::audioprocess))
         return true;
 
     return false;
 }
 
 } // namespace blink